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

Adds standardrb to ale #2133

Merged
merged 6 commits into from
Dec 10, 2018
Merged

Adds standardrb to ale #2133

merged 6 commits into from
Dec 10, 2018

Conversation

searls
Copy link
Contributor

@searls searls commented Dec 9, 2018

See: https://github.com/testdouble/standard ( standardrb/standard#43 )

StandardRB is to RuboCop what StandardJS is to ESLint. This commit
naively copies the RuboCop linter and fixer to point at the standardrb
executable. Any other adjustments are very minor (the only I can think
of is that standardrb takes a --fix option instead of
--auto-correct).

This raises a confusing point to me as both developer and a user: since
ale enables all linters by default, won't this run both RuboCop and
StandardRB (the results of which will almost always be in conflict with
one another)? How does ale already solve for this for the similar case
of StandardJS and ESLint?

In practice, it seems to "just work" and only run standard but I'm completely confused as to how, given that both rubocop and standardrb are present in the bundle. If someone could advise me on how this works I'd be happy to adjust things

See: https://github.com/testdouble/standard

StandardRB is to RuboCop what StandardJS is to ESLint. This commit 
naively copies the RuboCop linter and fixer to point at the standardrb
executable. Any other adjustments are very minor (the only I can think 
of is that standardrb takes a `--fix` option instead of 
`--auto-correct`).

This raises a confusing point to me as both developer and a user: since
ale enables all linters by default, won't this run both RuboCop and 
StandardRB (the results of which will almost always be in conflict with
one another)? How does ale already solve for this for the similar case
of StandardJS and ESLint?
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall, but it needs tests.

With regard to running both rubocop and standard, check your :ALEInfo to see what is enabled. If both are, rubocop is most likely just in agreement with standard; however, I think it is more likely that you do not have rubocop installed.

@searls
Copy link
Contributor Author

searls commented Dec 9, 2018

Re: testing, I read the developer guide on testing but I'm still too dense to know what you'd expect for a feature like this that invokes the methods in rubocop. Can you point to a similar example?

Re: running both, ALEInfo said both were enabled and both were running (per the command log below) but Rubocop was finding zero violations in each project I tested—this made no sense to me because running rubocop from the CLI would find a bunch

@neersighted
Copy link
Member

The tests should check the handler (error parser) against real (but hardcoded) output, and test that the command line parameters are built correctly. Check the existing tests (say, eslint or rubocop) for examples.

As far as running both and not getting any lints from rubocop, I would need to reproduce it myself to know what is going on.

@searls
Copy link
Contributor Author

searls commented Dec 10, 2018

Sorry that I'm struggling to add the tests you asked for:

  • I added a test for standardrb's error callback handler here

  • I added a test for standardrb's fixer callback here

  • Based on your advice, I looked for handler tests and so started with a duplicate of test_rubocop_handler.vader, but it wound up being virtually identical because my standardrb linter is configured to invoke the RuboCop handler directly, so I don't think tests in which we invoke the handler will add very much.

As for having both rubocop and standard installed, it seems to work fine in practice for me, but either way I am going to follow the example of standardjs (which is so closely analogous that it may as well be precedent) and just recommend standardrb users to explicitly specify what tools they want to run, as documented here

@searls
Copy link
Contributor Author

searls commented Dec 10, 2018

There was a test failure caused I think by an overly aggressive gitignore rule added in April here 7cf3ddf

It is ignoring all files that start with a ., recursively. For this reason my fixture file test/command_callback/ruby_paths/with_config/.standard.yml didn't get picked up by git and the build failed. I worked around this with git add --force, but I'm doubtful this was intentional. Maybe the gitignore rule should have been /.*?

w0rp
w0rp previously requested changes Dec 10, 2018
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests are pretty good now. See my comment here.

ale_linters/ruby/standardrb.vim Outdated Show resolved Hide resolved
@w0rp
Copy link
Member

w0rp commented Dec 10, 2018

The .gitignore file intentionally ignores all hidden files at every level, and you're right to use git add -f to add the file. I prefer doing that to people accidentally committing hidden files.

@w0rp
Copy link
Member

w0rp commented Dec 10, 2018

I added a comment to .gitignore just now, so people will understand that's the intention.

@neersighted neersighted merged commit 2cfa09e into dense-analysis:master Dec 10, 2018
@searls
Copy link
Contributor Author

searls commented Dec 10, 2018

Thanks!

@neersighted
Copy link
Member

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants