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

Add rubocop and standardrb formatters for Ruby #107

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

knu
Copy link
Collaborator

@knu knu commented Nov 22, 2020

Kudos for the multiple formatter support!

@lassik
Copy link
Owner

lassik commented Nov 22, 2020

Thank you very much for adding more formatters!

Can you make these changes:

  • Keep formatter names and definitions in alphabetical order (i.e. Ruby (rubocop, rufo, standardrb) and the like).
  • Extract the bundle exec code that is common to rubocop and standardrb into a utility function format-all--buffer-ruby. The formatter definition could then call (format-all--buffer-ruby "standardrb" "-a "-s").
  • The shell-command-to-string can probably be replaced with (with-temp-buffer (insert-file-contents ...) (re-search-forward ...)).

Would the existing Ruby formatter, rufo, benefit from using format-all--buffer-ruby as well?

@lassik
Copy link
Owner

lassik commented Nov 22, 2020

Hmm. We've had discussions of local-formatter support (e.g. #33) and bundle exec is kind of in that territory. We can go with a `format-all--buffer-ruby´ function for now, but should probably make a generic solution to project-local formatters later.

@knu knu force-pushed the add_rubocop_and_standardrb branch 2 times, most recently from a6f1914 to a85c60b Compare November 22, 2020 17:22
They require `--stderr` supported by rubocop >=1.4.0 and standard (standardrb) >=0.13.0.
@knu knu force-pushed the add_rubocop_and_standardrb branch from 7a23993 to 4a03c08 Compare November 22, 2020 17:35
@knu
Copy link
Collaborator Author

knu commented Nov 22, 2020

@lassik Thanks for the review! How does it look now?

@lassik
Copy link
Owner

lassik commented Nov 22, 2020

Thanks for putting a lot of effort into this!

What does ruby -ne 'print unless 1../^={20}$/'" do, and could we implement it in Emacs Lisp? That way we'd avoid depending on bash which isn't installed on BSDs and small Linux distros by default.

@knu
Copy link
Collaborator Author

knu commented Nov 23, 2020

It removes warnings that Rubocop generates from the output. Rubocop is basically a very noisy linter and it emits warnings to stdout where the formatted source code is also output. That's why I proposed adding --stderr, which got successfully merged. It will take some time until everyone starts to use a version of Rubocop with the --stderr option added, though.

As for processing within Emacs, the current internal API format-all--buffer-hard does not provide a way to process the output of a command, so I chose to do that with shell pipe. I also thought it'd be more performant when it's very common Rubocop emits a huge amount of warnings that is often longer than the formatted source code.

And yeah, I'm also the one with a BSD background who hates Bash-ism, but in reality, most popular ruby installation managers like rbenv and rvm already depend on Bash and I don't think it would be a big problem.

@lassik
Copy link
Owner

lassik commented Nov 23, 2020

The addition of --stderr is very good news - thanks for talking to upstream. If that feature is already out in a published version, we should go with it and leave out the hacks. If people encounter problems, could we just tell them to upgrade rubocop? format-all has no rubocop support so far, so it's not like we'll be breaking people's current workflows if we require the latest versio of rubocop.

@knu
Copy link
Collaborator Author

knu commented Nov 23, 2020

Yes, v1.4.0, the first release with the --stderr support seems to be rolled out today. Looks like they are preparing for v1.4.1 and I suspect v1.4.0 has a problem, though.

In any case, removing the hack too soon will make developers' lives hard until their projects get to upgrade Rubocop. I guess it'll be OK to force an upgrade in a couple of months from now.

@lassik
Copy link
Owner

lassik commented Nov 23, 2020

I'd prefer to avoid putting hacks into format-all as far as possible, since they easily get left in and it gets hard for people who didn't to write them to understand what they do and whether they're still relevant.

Since format-all currently has no Rubocop support, could we wait until 1.4.1 is published and working normally before adding Rubocop to format-all? If it only takes a month and we can avoid hacks, we can wait. If you need rubocop today, you can always use a private branch of format-all until then.

@lassik
Copy link
Owner

lassik commented Nov 23, 2020

(#42 is an earlier example where I opted not to merge a formatter since its interface was difficult to use reliably. One of the PHP formatters was another. These are pretty much the only ones; most formatter authors are happy to listen to feedback and provide a reliable interface in the next version.)

@lassik
Copy link
Owner

lassik commented Dec 11, 2020

@knu RuboCop is already at 1.6.1; it's 18 days since the release 1.4.0 that added the --stderr flag. Do you think we could rely on that flag?

@lassik
Copy link
Owner

lassik commented Feb 3, 2021

@knu Can you push a version of this patch that assumes RuboCop has --stderr support? We can probably rely on it by now.

@knu knu force-pushed the add_rubocop_and_standardrb branch from 4a03c08 to 3e6793a Compare March 1, 2021 13:55
@lassik lassik merged commit 5d920ed into lassik:master Jun 8, 2021
@lassik
Copy link
Owner

lassik commented Jun 8, 2021

Sorry, I hadn't noticed you had updated the patch. Thanks for your work! Now in master; can you check that it works?

@knu knu deleted the add_rubocop_and_standardrb branch June 18, 2021 22:03
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.

2 participants