Discuss Hound CI rules #4104

Open
seanlinsley opened this Issue Sep 1, 2015 · 25 comments

Projects

None yet

6 participants

@seanlinsley
Member

Hi everyone, it's been a long time since I've had the time to work on AA. But here I am!

Shortly after I left, @timoschilling added Hound CI to the repo. That's nice in theory, because it prevents reviewers from having to go through and comment on each style issue. However, I think (at least) the rules we currently have enabled make contributing a very unpleasant experience.

This screenshot from #4093 seems actively hostile towards the submitter. They're greeted with a barrage of comments from a robot, minutes after submitting their PR. And for everyone, it becomes nearly impossible to have an actual conversation on the PR because it's so hard to scroll down to it.

@seanlinsley
Member

I actually prefer single quotes.

# it signals that no interpolation is happening
'this is a static string'
# it works well for JSON
'{"foo": 42}'

But even if we were to reverse the setting in Hound, the opposite problem would still occur for those who submit PRs with double-quoted strings.

@seanlinsley
Member

It seems like a more human-friendly option would be to have a test that fails if the code doesn't follow our preferred style. That way there would be a failing test that the developer could run locally before resubmitting their code for review.

@scarver2
Contributor
scarver2 commented Sep 1, 2015

@seanlinsley Hi Sean. Thanks for the invite to this thread.

I prefer single quotes too even if there is no performance gain. The implied "this a string with no embedded logic" is easily recognized with single quotes. Can that feature be disabled in HoundCI? The standard does need to be consistent whatever is chosen.

On the plus side, I like that Tom added HoundCI. It shows that we are taking ActiveAdmin's development seriously.

More Preferable Instead of the barrage of HoundCI notifications, can we instead configure a common.rubocop.yml so that a developer can test locally before submitting a commit?

@deivid-rodriguez
Contributor

Hi! I agree sometimes HoundCI is too verbose.

I use https://github.com/brigade/overcommit in my own repos to try to enforce code style before PR's are submitted. Since that depends on contributors installing git hooks, I also run overcommit checks in travis so I get a failing build if a PR does not respect the repo's code style.

Regarding specific rules, I also prefer single quotes, but I don't really care as long as a consistent style is applied.

@javierjulio
Contributor

Haha @seanlinsley if you think that PR was bad, take a look at #3862 as it has 411 comments, most from HoundCI. Its a waste going through a PR fixing those Hound warnings, its easier to just run locally whatever tool Hound uses with ActiveAdmin's config or if we don't have a config (I added an SCSS one in PR #3862) then retrieve the default one from Hound. Its a lot to ask someone though to do this when they are most likely unfamiliar with the codebase. If we can have people contribute configs to tone down the warnings that'd be great. I have already contributed a custom SCSS config in PR #3862.

I'm not picky about preference as long as its consistent. Right now what the biggest issue is that the codebase doesn't adhere to Hound at all. It was added in without actually modifying it to fit what the maintainers would like the codebase to be or to best first its current state. So as of #3862 I'm getting tons of warnings on Ruby files I've only touched one part or two but I can't edit those since not only is out of scope for the PR but I doubt Hound was added without much thought to changing the config to best fit ActiveAdmin's current state.

@seanlinsley
Member

@javierjulio brings up a good point. Does anyone know if any of these tools have a command that will automatically update our code's style, as a one-time thing? That way the entire repo has a consistent style.

@timoschilling
Member

@seanlinsley The ' or " discussion is a big one. I think we should keep it by " while many projects are using the hound defaults.

Hound uses some different linters for the style checks. We can build a script which runs this linters locale.

@seanlinsley
Member

while many projects are using the hound defaults

We don't have any hard data to prove that one way or another.

And anyway, it doesn't matter what other projects do. The Rubygems style guide requires then after if, but I don't see anyone here suggesting we adopt that style.

@timoschilling
Member

I think it matters which style others projects use. If every project use a different one, it harder to contribute to new / many projects. If many projects use the same style, it's more easier.

@deivid-rodriguez
Contributor

RuboCop makes this very easy.

rubocop --autocorrect --auto-gen-config

automatically corrects offenses when possible and generates a "todo" config file with minimal allowed offense count limits, so that no offenses are triggered for the current state of the code. Not sure about the rest of the tools hound uses under the hood.

Regarding style, I also think it matters to adhere to commonly used practices, and to have good rationales when not following them. For me, the single one reference for Ruby is https://github.com/bbatsov/ruby-style-guide. Most rules are properly justified, it's followed by a lot of people, and it's community driven.

@deivid-rodriguez
Contributor

Also, note that with overcommit you don't need a custom script. overcommit --run runs all your linters locally. Maybe hound provides a similar feature?

@timoschilling
Member

I think there is one problem with https://github.com/bbatsov/ruby-style-guide, this guid don't have a option at some points. For example at the string literal quoting style.

(Option A) Prefer single-quoted strings when you don't need string interpolation or special symbols such as \t, \n, ', etc.

(Option B) Prefer double-quotes unless your string literal contains " or escape characters you want to suppress.

Hound uses option B, and I think the most projects that uses hound will use the default hound config. (I will make a statistic over this)

@deivid-rodriguez
Contributor

Well, I consider the single vs double quote debate a waste of time. There's no consensus, so just follow any rule you want. Rubocop provides it as a config option, so it's just a matter of changing one line in the config and adjusting the style. And I assume Hound uses RuboCop under the hood.

Regarding the Ruby Style Guide, it's just one style guide (not a list of style guides), so it needs to be opinionated by definition.

@timoschilling
Member

@deivid-rodriguez I think overcommit is the wrong tool. We need something like 'hound-cli' or 'multi-linter' (they don't exist, but they should.)

@deivid-rodriguez
Contributor

Overcommit works great for me, but again, I'm fine with any tool as long as one is used. I just find Hound CI and this "robot comment thing" a bit too verbose and invasive.

@scarver2
Contributor
scarver2 commented Sep 1, 2015

@deivid-rodriguez agreed. I'm more concerned about real offenses than single quotes.

As aggravating as Rubocop is about pointing out our fallacies, it certainly unifies our syntax.

I like overcommit in theory, but wouldn't it block commits that are works-in-progress? I wouldn't want to push away a new contributor, because they couldn't get a commit to pass lint tests. They will be punished enough by the Hound CI messages :)

@deivid-rodriguez
Contributor

Overcommit checks can be skipped, not installed at all, run only on push, etc. It's very configurable. But I understand your concern. Unfortunately, I don't think my repos give us useful information about that. byebug had a low contribution rate before and after adding overcommit.

Maybe the easiest solution for now is to get rid of Hound and add the rake tasks provided by RuboCop, SCSSLint and the rest of the linters to the build and the test suite.

@scarver2
Contributor
scarver2 commented Sep 1, 2015

@deivid-rodriguez Thanks for the A/B test info. I'm not opposed to overcommit. I'm sure we can document how to use overcommit in the CONTRIBUTING.md file.

@timoschilling In case you don't know, StringLiterals can be turned off in Rubocop. There is an open issue to make the StringLiterals configurable. In the meantime there is this:

StringLiterals:
  Enabled: false

QUESTION: Is it possible for an administrator to delete all the HoundCI auto-comments from an issue to get rid of all the noise?

@deivid-rodriguez
Contributor
deivid-rodriguez commented Sep 1, 2015 edited

@scarver2 That issue has been closed for almost two years. The feature is already there! :)

@seanlinsley
Member

QUESTION: Is it possible for an administrator to delete all the HoundCI auto-comments from an issue to get rid of all the noise?

There isn't any way to do that from the UI. We could potentially write a script that uses the Github API.

@seanlinsley
Member

The hundreds of Hound comments on PRs were getting in my way when trying to review them, so for now I've disabled Hound.

@javierjulio
Contributor

Yes thank you @seanlinsley! 👍🏻 While it is very helpful its no good unless whoever adds Hound runs it first and updates code to meet its requirements. This would be a great PR to do for each language we enable checks on. I realized when working on my PR that no one had run it against the existing codebase since all these warnings came up as you submitted commits. It was rather demoralizing for me. For now this is best as it will help to keep contributions coming in.

@scarver2
Contributor

Thanks @seanlinsley

@varyonic varyonic added the discussion label Jan 22, 2017
@varyonic
Member

I like David's rubocop --autocorrect --auto-gen-config as a way forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment