Linting Upgrade

We've been working on updating the .eslintrc file to extend the airbnb config
rather than having a list of rules we manually maintain. Anytime you change
the style configuration in a project there are some challenges and
decisions.

The first is what style guide to adopt. The airbnb eslint config has become
a standard in the community and enables a simplified .eslintrc file. It
definitely seems like the right approach.

The second is whether or not to update all files at once or let files be
changed over time as they're worked on. There are advantages and
disadvantages to updating all files at once.

Advantages

  • You can see the affect of all the new lint rules at once to make sure
    the configuration is correct and there are no rule conflicts.
  • All the files are formatted to pass linting rules.
  • Subsequent PRs don't include these style updates which makes them
    easier to understand.

Disadvantages

  • The PR is quite large.
  • It can take a lot of work to get all the files matching the rules.
  • Rebasing after this PR could be a minor challenge depending on what
    you're working on.

Moving forward

My opinion is to go with updating all the files to get the advantages
stated above. If anyone has any feedback or insight into this process feel
free to chime in.

We'll also likely be introducing prettier to handle the formatting side of
linting once airbnb is in place so this will serve as a bit of a test run.

Thanks!

I've done some more research on the scope of these changes and it looks
like there are about 150 files that can't be fixed automatically with a
little over 700 errors total. Here are the common issues and my estimation
of how hard they are to fix.

import statements - 219 errors - easy

react/props - 226 errors - medium/hard
Adding prop types can be time consuming and if the prop-types are not
required then default prop-types need to be supplied

no-* (underscore-dangle, use-before-define) - 110 errors - medium
Many of these are easy but some are deeper than the one file with the lint
error. For example the no-underscore-dangle could be in lots of files and
is a convention used to separate the variable name. We may need to ignore
these errors either in the files themselves or just turn the rule off
entirely.

Making these changes would definitely improve the code base but it may not
make sense to do them all at since the workload is large and many changes
will take some research.

··· On Tuesday, November 7, 2017 at 11:17:14 AM UTC-5, dsee...@redhat.com wrote: > > We've been working on updating the .eslintrc file to extend the airbnb config > rather than having a list of rules we manually maintain. Anytime you change > the style configuration in a project there are some challenges and > decisions. > > > The first is what style guide to adopt. The airbnb eslint config has > become a standard in the community and enables a simplified .eslintrc file. > It definitely seems like the right approach. > > > The second is whether or not to update all files at once or let files be > changed over time as they're worked on. There are advantages and > disadvantages to updating all files at once. > > *Advantages* > > - You can see the affect of all the new lint rules at once to make > sure the configuration is correct and there are no rule conflicts. > - All the files are formatted to pass linting rules. > - Subsequent PRs don't include these style updates which makes them > easier to understand. > > *Disadvantages* > > - The PR is quite large. > - It can take a lot of work to get all the files matching the rules. > - Rebasing after this PR could be a minor challenge depending on what > you're working on. > > > *Moving forward* > > My opinion is to go with updating all the files to get the advantages > stated above. If anyone has any feedback or insight into this process feel > free to chime in. > > We'll also likely be introducing prettier to handle the formatting side of > linting once airbnb is in place so this will serve as a bit of a test run. > > Thanks! >

Hi,
I think using a generally accepted style guide is a better approach than
creating our own from scratch. I would just like to point out that it does
not remove the challenges entirely as new versions of style guide are
released and new rules added, but that is just something we have to deal
with, so +1 from me.

Whether to update all files in one PR is ultimately a question of how much
time we want to invest in it right now. If there are no pressing issues at
the moment, then we could update everything. But if it seems like it would
be too much of a time commitment, then lets fix the easy ones and leave the
difficult ones to be fixed when we work in that area later.

O.

··· On Tue, Nov 7, 2017 at 5:56 PM, wrote:

I’ve done some more research on the scope of these changes and it looks
like there are about 150 files that can’t be fixed automatically with a
little over 700 errors total. Here are the common issues and my estimation
of how hard they are to fix.

import statements - 219 errors - easy

react/props - 226 errors - medium/hard
Adding prop types can be time consuming and if the prop-types are not
required then default prop-types need to be supplied

no-* (underscore-dangle, use-before-define) - 110 errors - medium
Many of these are easy but some are deeper than the one file with the lint
error. For example the no-underscore-dangle could be in lots of files and
is a convention used to separate the variable name. We may need to ignore
these errors either in the files themselves or just turn the rule off
entirely.

Making these changes would definitely improve the code base but it may not
make sense to do them all at since the workload is large and many changes
will take some research.

On Tuesday, November 7, 2017 at 11:17:14 AM UTC-5, dsee...@redhat.com > wrote:

We’ve been working on updating the .eslintrc file to extend the airbnb config
rather than having a list of rules we manually maintain. Anytime you change
the style configuration in a project there are some challenges and
decisions.

The first is what style guide to adopt. The airbnb eslint config has
become a standard in the community and enables a simplified .eslintrc file.
It definitely seems like the right approach.

The second is whether or not to update all files at once or let files be
changed over time as they’re worked on. There are advantages and
disadvantages to updating all files at once.

Advantages

  • You can see the affect of all the new lint rules at once to make
    sure the configuration is correct and there are no rule conflicts.
  • All the files are formatted to pass linting rules.
  • Subsequent PRs don’t include these style updates which makes them
    easier to understand.

Disadvantages

  • The PR is quite large.
  • It can take a lot of work to get all the files matching the rules.
  • Rebasing after this PR could be a minor challenge depending on what
    you’re working on.

Moving forward

My opinion is to go with updating all the files to get the advantages
stated above. If anyone has any feedback or insight into this process feel
free to chime in.

We’ll also likely be introducing prettier to handle the formatting side
of linting once airbnb is in place so this will serve as a bit of a test
run.

Thanks!


You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.