Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could view linting be suppressing contributions? #1828

Open
benhalpern opened this Issue Feb 18, 2019 · 18 comments

Comments

Projects
None yet
8 participants
@benhalpern
Copy link
Collaborator

benhalpern commented Feb 18, 2019

I have the feeling that view linting could be presenting a pretty strong burden to contributions. You could make a small contribution and get hung up with a huge linting fix chore as it currently stands.

I just feel like as an outsider without context it could add a bit too much burden at the time being. Thoughts on taking it away as a blocker to pushing and leave it as something folks can voluntarily make progress on until the app is at a closer place to being fully linted on the view with our preferred styles?

@triage-new-issues triage-new-issues bot added the triage label Feb 18, 2019

@triage-new-issues triage-new-issues bot removed the triage label Feb 18, 2019

@rhymes

This comment has been minimized.

Copy link
Contributor

rhymes commented Feb 19, 2019

You make a really good point!

I don't know if it's actually deterring people from contributing but I agree that it can be a nuisance when ran automatically, especially if the contributor is a beginner.

I wonder if, before taking a decision either way, we could run the linting on the view files and see how far along the whole thing is and/or if Code climate has a specific metric on those.

This way it can be decided that until it reaches, for example 90+% of linting, it gets disabled as a git hook, to be re-enabled afterwards... ?

The downside of disabling view linting altogether is that people need to open a PR to fix linting, either with small mergeable increments or a quick one off. Because linting only PRs kept open too long tend to conflict with feature changes.

@lightalloy

This comment has been minimized.

Copy link
Collaborator

lightalloy commented Feb 19, 2019

@benhalpern, you mean that someone makes a small contribution and has to fix all the issues in the changed file? That could be a problem (though I'm not sure about it).

In my opinion, the code will never reach 90+-% (or similar) of linting w/o the hook that forces the "correct" code style. Because this way the new code won't be checked also, and the contributors (even the core team) won't check it every time unless the check is forced. So the gradual fixing from time to time could be an endless process, and so won't be effective.

One of the solutions could be fixing all issues at once, but this solution has its own disadvantages: spending significant time on this and possible conflicts that would need to be fixed.

@Glennmen

This comment has been minimized.

Copy link
Contributor

Glennmen commented Feb 19, 2019

I think Ben is referring to what I mentioned here #1784 and that was only from one view file.

Also I wrote a little about it in my blog post and tried to open a discussion about it, glad to see that Ben is doing that here.

@lightalloy

This comment has been minimized.

Copy link
Collaborator

lightalloy commented Feb 19, 2019

@Glennmen Thanks for starting a discussion.
As for the existing linting issues in views, there are a lot of them, cause views linting has been introduced recently. The hook should only check the new or changed files before the commit. For now, I'm not sure why it required you to fix an untouched view file.
The --no-verify is probably used sometimes. In my opinion, it's not ok in most cases. I've noticed that several new rubocop issues appeared since I have checked (and fixed) the last time. In my opinion, it's better to add exceptions to the rubobop_todo or use rubocop:disable/enable at least, than run with --no-verify.

@Glennmen

This comment has been minimized.

Copy link
Contributor

Glennmen commented Feb 19, 2019

@lightalloy Just to be clear, I did edit that view file. I added the Github PR info to the markdown help docs.
However I still had to add --no-verify or else I wasn't able to commit, there was 1 linting error that I wasn't able to fix because it would parse the html in the code block instead of print it and I wasn't able to find a fix for that.

@lightalloy lightalloy referenced this issue Feb 19, 2019

Merged

Disable erblint cops for the editor guide #1830

1 of 1 task complete
@lightalloy

This comment has been minimized.

Copy link
Collaborator

lightalloy commented Feb 19, 2019

@Glennmen, oh, I see. Thanks for the explanation. I've made a pr to disable the corresponding cops.

@nickytonline

This comment has been minimized.

Copy link
Contributor

nickytonline commented Feb 19, 2019

I have no idea if it's preventing contributions. Could be a good meta post @benhalpern on the site to ask the community.

Having said that, I don't think the issue is the pre-commit, but the fact that there is linting debt and running --no-verify adds to that. If someone were to create new files, so no previous lint debt, and linting fails, I consider that a good thing. The linting rules (eslint, rubocop) are in place on the front-end and back-end to help make the code more maintainable.

The other thing is the pre-commit tries to fix linting errors, hence the --fix, so if there are still other errors, it's really because linting rules were violated.

I would approach it this way:

  • Look at the existing rules. Are we happy with them? Do we want to drop some?
  • Assess the lint debt, i.e. which files fail linting currently.
  • Fix them (could be a big job potentially), but an offshoot of this is these are actually potentially good first issues. (but if linting is an issue to contributing, then infinite loop 😉)

I think enforcing these rules, assuming we fix existing linting debt is a good thing. It's not uncommon in the real world (open source or closed source) to fail at the pre-commit or PR build stage because of linting.

Just tor reiterate, this codebase will probably live a long life, so whatever we can do to keep it maintainable in an automated way is a good thing.

@benhalpern

This comment has been minimized.

Copy link
Collaborator Author

benhalpern commented Feb 19, 2019

@nickytonline I made a post to ask the question in general. It could add to the discussion here. https://dev.to/ben/can-forced-linting-surpress-contributions-when-linting-is-first-introduced-2dn5

I agree with the longterm benefits of linting, of course, but I feel like we may want to introduce this particular hook in some kind of more gradual way. Perhaps as a voluntary option at first.

Anyway, I’m not super strongly opinionated on this, I just wanted to get the concern out of my head. I’ll let @lightalloy and @maestromac make whatever choice they feel is appropriate. No rush.

@Glennmen

This comment has been minimized.

Copy link
Contributor

Glennmen commented Feb 19, 2019

I already planned to make an issue about something else I found before this issue but since it is kinda related I will add it to the discussion here (if wanted I could still make a separate issue).
This was something that wasn't being catched by the pre-commit linting, travis, codeclimate, ... but I only noticed it because of my IDE (RubyMine/JetBrains). None of the users probably noticed because luckily browsers are very "forgiving' and are able to parse the HTML even with errors.

Besides the linting errors that are being thrown now there are tons of HTML errors in loads of view files, some examples:
Different opening and closing tags:

No <dl> closing tag (multiple times in the same file):

<dl>
<dt><code>Custom tabs</code></dt>
<dd>
You can add a custom tab order to you JSFiddle embed tag. Defaults to <i>js,html,css,result</i><br>
<code>{% jsfiddle https://jsfiddle.net/webdevem/Q8KVC result,html,css %}</code>
</dd>

No <h4> closing tag:

<h4>This tool was created with love by Charles Berlin (<a target="_blank" rel="noopener" href="http://twitter.com/AModelEngineer">@AModelEngineer</a>)

I think you get the picture 😉 I would like to go through the files in /app/view/* and fix any HTML errors (and linting errors while I'm at it). But need to know a couple of things first:

  • How to submit?
    • 1 Big PR
    • Separate PR's for each folder (combined by first subfolder, nested folders included in that PR)
  • Linting errors that I can't fix?
    • Ignore them
    • Notify someone from DEV? to change lint rules and merge master to my branch to commit without errors.

Currently these are the only questions that I can think of.

So what you guys think?

@aspittel

This comment has been minimized.

Copy link
Collaborator

aspittel commented Feb 19, 2019

I would lean towards disabling this until refactoring/cleaning up of older code is complete. Having a bunch of errors intimidated me when I started working on the code base, and I think it might for other people too. Refactoring some of the Preact code to fit the linting requirements would be pretty time consuming with the amount of errors and the depth of them.

@maestromac

This comment has been minimized.

Copy link
Collaborator

maestromac commented Feb 19, 2019

I like @nickytonline 's approach a lot.

@Glennmen 1 big PR for fixing lint is fine but please split it up if it's more than 1000 line changes. Please ignore what you can't fix but also let us know what those are so we can decide if something should be changed.

@Zhao-Andy

This comment has been minimized.

Copy link
Collaborator

Zhao-Andy commented Feb 19, 2019

Also in favor of disabling until a refactoring/linting is done, like @aspittel said. Following @nickytonline's approach seems to be the right direction too.

It might be worthwhile to look at this from an 80/20 perspective, where we spend some time to resolve linting errors for the easiest 80% of the files (all .scss and .html.erb files, for example), and leave the hardest 20% to be refactored properly (Webpack/Preact/.jsx files, for example) another time. .js.erb (Javascript in asset pipeline), .js and .jsx (Webpack) files will require more time and consideration to refactor properly.

I believe @lightalloy and @rhymes (and probably others I'm forgetting, sorry!) refactored a bunch of cops for Ruby files already, and we have a nice .rubocop_todo.yml file to work off of now. I think resolving the HTML and SCSS files will be pretty straightforward.

@rhymes

This comment has been minimized.

Copy link
Contributor

rhymes commented Feb 19, 2019

I've tried running erblint --lint-all --autocorrect on the entire code base and I got the following output:

$ time erblint --lint-all --autocorrect
Linting 278 files with 11 autocorrectable linters...
[...]

# HERE THERE IS A LIST OF FILES THAT CAN'T BE AUTOCORRECTED

2010 error(s) corrected in ERB files
107.93s user 4.71s system 119.33s real 84308kB mem

The git diff is quite big and completely generated by the autocorrect. Mostly spaces, fixed indentation, hash parameters and other small things but I also see some questionable refactorings.

This is how many lines have been changed:

$ git diff --shortstat
 199 files changed, 1460 insertions(+), 1521 deletions(-)

I would not recommend doing a big PR with the autocorrect, but maybe limiting the scope to a small amount of files for each PR should be doable. So that one can manually test if there's anything broken in the pages.

I've attached the output of git diff --patch if you want to have a look

erblint.patch.txt

@Glennmen

This comment has been minimized.

Copy link
Contributor

Glennmen commented Feb 19, 2019

From rhymes his intel I would say it might be to big for 1 PR, especially since those numbers are only lint fixes and doesn't include any HTML fixes that I also want to include in the PR.

@lightalloy

This comment has been minimized.

Copy link
Collaborator

lightalloy commented Feb 20, 2019

Please, be careful with --autocorrect. It's not ideal for erblint, I found out that sometimes it tries to fix the indentation incorrectly, which leads to invalid code. So the autocorrected files should be checked before committing.
I agree with @rhymes that it would be better to make relatively small prs to eliminate the debt gradually,

@Glennmen

This comment has been minimized.

Copy link
Contributor

Glennmen commented Feb 20, 2019

I will probably start fixing the view files this weekend, unless someone decides it needs more thought first.
Because of the size of the changes this is my planned workflow:

  • Each folder in /app/views/ will be a separate PR, so 32 PR's will be made. Some might be small PR's with only a couple of changes but splitting this up will help us review quicker and get it all merged.
  • I will only do html.erb files, so no javascript or json files if there are any in the views folder.
  • I will not do --autocorrect, I am going to go through each file manually and fix any linting and html errors.
  • If there any linting errors that I can't fix, I will mention it in the PR draft/WIP and only after all errors are fixed I will make it a release ready PR.

Any feedback from you guys is welcome, especially to keep it easy for you guys to review it.

@lightalloy

This comment has been minimized.

Copy link
Collaborator

lightalloy commented Feb 21, 2019

@Glennmen Using --autocorrect is fine, if you check the fixes manually afterward. It fixes most of the cases correctly and could make the process much faster.

@lightalloy

This comment has been minimized.

Copy link
Collaborator

lightalloy commented Feb 21, 2019

@Glennmen I've also made an issue for the task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.