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

[Discussion] Direction of rufo #2

Closed
bessey opened this issue Jul 16, 2017 · 30 comments
Closed

[Discussion] Direction of rufo #2

bessey opened this issue Jul 16, 2017 · 30 comments

Comments

@bessey
Copy link
Member

bessey commented Jul 16, 2017

I am keen to discuss where rufo is heading, so I thought i'd start an issue on the topic.

Personally I am very excited about this project, and am keen to see its continued development. I'm a little tired of spending time in code reviews paying attention to formatting concerns, to me, this is solely the job of a formatter, and the more opinionated the better. I'd like my formatter to enforce rules strictly, even if I don't necessarily agree with them. The value I get from there being a One True Format exceeds that of having code look just how I like it.

To that end, I'd like to see the default settings of rufo be more opinionated, and less permissive. Much like tools like prettier. But that's definitely my personal bias, and I'd love to know what others think on the matter.

@danielma
Copy link
Member

In short, I feel very much the same. I want an opinionated formatter for the One True Format.

In fact, I have a block of time at work (August 7-18) where my goal is to be able to prsent a "prettier for Ruby" to my company by the end. I feel like rufo is in a solid place now, with line length support as the major required feature.

@splattael
Copy link
Member

splattael commented Jul 16, 2017

+1 for an opinionated formatter.

I love how elm-format is not configurable at all.

As for "One True Format" in Ruby. We should not forget the "Seattle Style" (omitting parens where as much as possible). Introducing a single configuration option seattle_style true would work for me ;)

@mjago
Copy link
Member

mjago commented Jul 16, 2017

I'm in 👍. Actually I've been reading up on Prettier over the last couple of days since I'm not familiar with it and was sharing your thoughts. The take-up of Prettier seems to have been quite remarkable precisely because it makes the decisions for you (or the team).

It will also be much easier to get to a stable codebase this way (the remaining bugs that are known about relate to clashing styling options for instance). Much easier to test too. For me consistency is the most important style anyway. I guess it won't suit everyone but they have alternatives.

So we should discuss the one true format.

@bessey
Copy link
Member Author

bessey commented Jul 16, 2017

Well I'm glad others are in agreement that there should be one true format, but yes, now we need to agree on what it is... Let me take a swing based on the current settings rufo has to offer...

align_assignments: real false (enforce NOT aligning, my view is the git noise this generates when adding new long variable names outweighs that gained from aesthetics).
align_case_when: no opinion (I've actually never written a case statement in this when then style
align_chained_calls: none of these options seem right to me, I would prefer enforcing a newline for even the first call, and one indent level, but only when the line crosses your max line length. E.g:

MyChainableClass
  .scope_1
  .scope_2

align_comments: false, don't modify.
align_hash_keys: ooof, we do this all the time at my company and its a pain manually but very easy when automated, so i guess true.
double_newline_inside_type: dynamic seems fine
indent_size: 2
parens_in_def: yeah... this one is always going to need an option, you'll never get the ruby community to agree on this :P. For me it would be parens only when there is more than 1 arg.
spaces_after_comma: one
spaces_after_lambda_arrow: one
spaces_after_method_name: no (I've also never seen ruby written like this though, so dont mind so much)
spaces_around_binary: dynamic, I think there's value here both for aesthetics and hinting at application order
spaces_around_block_brace: one
spaces_around_dot: no
spaces_around_equal: one
spaces_around_hash_arrow: one
spaces_around_unary: no
spaces_around_when: one
spaces_in_commands: one
spaces_in_suffix: one
spaces_in_ternary: one
spaces_inside_array_bracket: never
spaces_inside_hash_brace: never
spaces_in_inline_expressions: one
trailing_commas: dynamic
visibility_indent: align

@danielma
Copy link
Member

@bessey thanks for writing this up. Overall, I agree with you. A few small exceptions:

parens_in_def: yeah... this one is always going to need an option, you'll never get the ruby community to agree on this :P. For me it would be parens only when there is more than 1 arg.

I think this is one of the few settings that deserves to have an option. As @splattael mentioned, seattle style has enough mindshare in the community that's it would benefit rufo to support it. Personally, I say parens_in_def :always

spaces_around_binary: dynamic, I think there's value here both for aesthetics and hinting at application order

Personally, I would say :one, and suggest the use of parens to hint at application order.

spaces_inside_hash_brace: never

Personally, I use :always, but that is purely a style thing. I would be fine with either decision.

trailing_commas: dynamic

I would say always. I think a code formatter should attempt to reduce the amount of diff-churn if possible.

In addition, to address @mjago's comment

no tabs please I'm Emacs 😉

I think rufo should support both the use of spaces and tabs for indentation. In terms of implementation, it's trivial to support both, but I feel this could be a boon for adoption if there is a team/individual that feels they must use tabs.

@bessey
Copy link
Member Author

bessey commented Jul 17, 2017

Hey Dan, for me all of your examples fall firmly in the "i don't mind as long as we all do the same thing" camp, so happy to go with your preferences as defaults there. Not sure what the fairest way is to decide on these things is, without users to survey. I wonder if there is existing open data to consult on this matter?

PS. The members of this org are all private so I have no idea which of you (if any, haha) actually controls this fork. Any chance of making that public?

@danielma
Copy link
Member

danielma commented Aug 7, 2017

Status update for you guys: I have time this week and next at work to work on getting a ruby formatter into the wild, and I'm hoping to work on top of the existing rufo code base. So hopefully we'll have a flurry of activity for the next few weeks. My major goals are

  • re-work tests to be more human readable
  • remove a bunch of settings (as we described in this issue)
  • implement line-length support

@mjago
Copy link
Member

mjago commented Aug 8, 2017

@danielma Just a quick point - that quote of me was actually a poor attempt at a Vim / Emacs joke, and I have since removed the comment. It certainly wasn't any suggestion that tabs would be a good thing - and personally I would fight any attempt to allow tabs in a "one size fits all" specification. Tabs are a relic from word-processor days and no sane presentation layer or coding standard permits them. Indeed most (all?) modern editors use the concept of indent completely apart from embedded "\t" symbols. For accessibility purposes (poor eyesight) any good editor can indent to 8 or 12 on the fly and I suspect that use-case is tiny anyway (but real - I have worked with such a challenged programmer - but he was not an advocate for tabs).

@martinos
Copy link

martinos commented Oct 4, 2017

I would say that I was very surprised there was a trailing_commas. I think that many rubyist don't even know that you can put a trailing comma at the end of an array or a hash.

I vote for no trailing comma since this what is use almost everywhere and also this is the Rubocop default.

I am wondering what is the reason allowing a trailing comma? Is it to clean the diffs, if so personally I think this is not a strong argument since these diffs are very easy to understand. The only reason that I see is that it makes it easier to move elements around.

I think that if Rufo becomes the defacto standard it should not make too many changes to the current standard so that when people start using it they don't get too many format diffs.

I think that making Rufo non configurable is a very good idea, however I think that we should be as close to Rubocop as possible or at least we should give a sample .rubocop.yml file in the documentation.

gingermusketeer pushed a commit to gingermusketeer/rufo that referenced this issue Oct 5, 2017
* feat(backtick_strings): Format backtick_string & %x()

* chore(NewFormatter): clean up

Just a few bits of cleanup for consistency.
@bessey
Copy link
Member Author

bessey commented Oct 6, 2017

@martinos

I would say that I was very surprised there was a trailing_commas. I think that many rubyist don't even know that you can put a trailing comma at the end of an array or a hash.

Agreed, I was aware Ruby was capable of this, but I haven't written Ruby code this way until I started using Rufo. To me the benefit is not purely diff reduction, but the follow-on effect: accurate git blame.

That said, I am easy on this, I see the benefit to having "normal" looking lists and maintaining the more common existing convention, and equally the benefits of improving my git life (I use blame a LOT, even if its just to prove it was me that wrote that bad code :P)

@martinos
Copy link

martinos commented Oct 6, 2017

@bessey, I never thought about that. Personally I haven't used git blame for many years, just because of that kind of reason and also because you just see the current status of the file. I am a very big fan of git log --patch. Once I launch it (in vim), I search for a string in the code that I am looking for. It is almost as fast and way more reliable and informative.

Thanks for taking time to answer me, it's always nice to have another person's opinion. But I am like you, I don't really care what is the default that is chosen, formatters are so useful. The only think is that I prefer that it doesn't fight too much with Rubocop since it might stop Rufo adoption which I think I more useful than Rubocop.

@gingermusketeer
Copy link
Member

I have created a PR to change the config related to the trailing_comma behaviour. Let me know what you think #35.

@bessey
Copy link
Member Author

bessey commented Oct 19, 2017

To summarise for new arrivals (and myself to be honest!), we are now down to just these 6 configurable settings on master. I've included my understanding of the current plan for each of them, based only on previous comments and merged PRs:

  1. align_case_when: Undecided
  2. align_chained_calls: Undecided
  3. double_newline_inside_type: Undecided
  4. parens_in_def: Remains, now defaults to yes
  5. spaces_around_binary: Remains, still defaults to dynamic
  6. trailing_commas: Remains, now defaults to yes

@bessey
Copy link
Member Author

bessey commented Oct 19, 2017

I think its worth discussing 1, 2, 3, and 5 further, though I suggest we create an issue for each.

@mjago
Copy link
Member

mjago commented Nov 1, 2017

spaces_around_binary: [:dynamic, :one]
parens_in_def: [:yes, :dynamic]
double_newline_inside_type: [:dynamic, :no]
align_case_when: [false, true]
align_chained_calls: [false, true]
trailing_commas: [:always, :never]

I wonder if we should harmonise the settings values a little, now that we have such a small subset remaining. For instance it would be nice if trailing_commas was [true, false] rather than [:always, :never]. And could dynamic settings become [:dynamic, :fixed], and described in the Settings page ?

This would leave us with three [true, false] settings and three [:dynamic, :fixed] settings. Much easier to remember whilst crafting your .rufo file 😉

@gurgeous
Copy link
Contributor

I love this project and the direction that you are going with removing config! I might make one suggestion - when uncertain about a style decision, it would be interesting to consider what prettier does.

For example, prettier turns this:

let x = [1,2,3,]

into this:

let x = [1, 2, 3];

(no trailing commas)

@bessey
Copy link
Member Author

bessey commented Jan 25, 2018

Thanks for the support :) time to ease back into maintenance after a nice Xmas break I think...

To your specific example, I don't think anyone envisioned trailing_commas applying to one liners, as the benefit of them is only really observed with things like reordering or appending to multiliners:

let x = [
  1,
  3,
  2  #can't append to without modifying, can't reorder either
]

@gnapse
Copy link

gnapse commented Mar 28, 2018

I just want to say I just found this project and I love it. I think I finally found my prettier-for-ruby. I love the "minimum configurable stuff" mindset that I see here.

Just one question though (perhaps a topic for a separate issue so not to pollute this one but I'm also hesitant of opening yet another issue): why not integrate this project with prettier itself? There's a very young effort of achieving a prettier-ruby and maybe rufo is the answer for this, at least the technology behind rufo (its parser and lexer).

Update: Never mind the question about contributing with prettier-ruby. I just found out there's already a thread discussing this in their repo.

@BrianHawley
Copy link

@martinos I don't like matching the Rubocop defaults, as they made several bad choices in their defaults. However, I do agree that rufo should have a sample .rubocop.yml file that configures Rubocop to not undo any of the rufo changes. My main concern is having a set of default settings in my projects that will minimize the problems caused when other developers use Rubocop.

I know about the difference in quoting, but I don't know what the other differences are. If there was a list of the rufo rules, we could make a corresponding set of Rubocop rules to match, or maybe a Rubocop plugin gem that defines those rules automatically.

@BrianHawley
Copy link

It would be nice to have a trailing_commas setting (or unsettable default) which would not add trailing commas to keyword parameters in an argument list of a method call. It would be sufficient to not add a trailing comma in cases where the hash is not wrapped in { and }, or when the hash or array is all on one line.

@ndbroadbent
Copy link

ndbroadbent commented Nov 15, 2018

@BrianHawley I want to use Rufo and RuboCop at the same time. Would it be possible to work with the RuboCop team so that your default config enforces the same style? What do you think @bbatsov?

If we go by number of GitHub stars, I think RuboCop and the Ruby Style Guide should be the decider. I don't like some of the rules either, but the fragmentation of these projects is far worse.

I do like the choice to remove configuration. Maybe as a short-term workaround you could link to an official Rufo RuboCop configuration file that people can use? This way, people could easily configure RuboCop to play nice with Rufo, since there's no way to configure Rufo.

EDIT: I just saw the comment from @martinos about including a sample .rubocop.yml file in the documentation. I strongly agree with that. It's the main reason why I haven't yet adopted rufo in my own project.

@bbatsov
Copy link

bbatsov commented Nov 15, 2018

I'm always open for collaboration, but given the fact that RuboCop is both a linter and a formatter I can't imagine someone is going to use it just for linting. Just sounds weird to opt to use two tools when you can use just one.

@martinos
Copy link

martinos commented Nov 15, 2018

The main reason for using Rufo with Rubocop is that Rufo is way faster. Thus you can run Rufo on save and run Rubocop before committing and fix other linting issues. But since the output syntax differ for both tools, they fight each other which is far from ideal. I think that the slow adoption of Rufo might be caused by that reason.

I've shown Rufo to some devs and since it's output syntax diverge most standards they were reluctant to use it. Anyways, I still use it because I am the only dev in my projects and I am very satisfied.

@ndbroadbent
Copy link

ndbroadbent commented Nov 15, 2018

I just tried out RuboCop and Rufo on a badly formatted Ruby hash with a really long line:

a = {
  foo: 123,
    bar: 23, baz: 23,
        yes: 1231232132, asfasdfasdfasdfas: 342342343243, asdfasdfasdfsdfas: 324, asdfasdfasdfadsfasdfdsf: 23423432,



}

RuboCop

Output

$ cp test.rb test.rubocop.rb; time rubocop -x test.rubocop.rb
Inspecting 1 file
C

Offenses:

test.rubocop.rb:3:5: C: [Corrected] Layout/AlignHash: Align the elements of a hash literal if they span more than one line.
    bar: 23, baz: 23,
    ^^^^^^^
test.rubocop.rb:4:9: C: [Corrected] Layout/AlignHash: Align the elements of a hash literal if they span more than one line.
        yes: 1231232132, asfasdfasdfasdfas: 342342343243, asdfasdfasdfsdfas: 324, asdfasdfasdfadsfasdfdsf: 23423432,
        ^^^^^^^^^^^^^^^
test.rubocop.rb:6:1: C: [Corrected] Layout/EmptyLines: Extra blank line detected.
test.rubocop.rb:7:1: C: [Corrected] Layout/EmptyLines: Extra blank line detected.

1 file inspected, 4 offenses detected, 4 offenses corrected

real	0m2.511s
user	0m1.868s
sys	0m0.625s

Result:

a = {
  foo: 123,
  bar: 23, baz: 23,
  yes: 1231232132, asfasdfasdfasdfas: 342342343243, asdfasdfasdfsdfas: 324, asdfasdfasdfadsfasdfdsf: 23423432,

}

Rufo

Output:

$ cp test.rb test.rufo.rb; time rufo test.rufo.rb
Format: test.rufo.rb

real	0m0.213s
user	0m0.159s
sys	0m0.049s

Result:

a = {
  foo: 123,
  bar: 23, baz: 23,
  yes: 1231232132, asfasdfasdfasdfas: 342342343243, asdfasdfasdfsdfas: 324, asdfasdfasdfadsfasdfdsf: 23423432,

}

Honestly I'm a bit disappointed that neither one has an opinionated way of splitting up the hash into multiple lines. And I don't like that empty line at the end of the hash. I want my code formatter to produce well-formatted hashes and arrays, and know how to split up method arguments into multiple lines, etc.

Here's how Prettier does it:

Before:

const a = {
  foo: 123,
        bar: 23, baz: 23,
              yes: 1231232132, asfasdfasdfasdfas: 342342343243, asdfasdfasdfsdfas: 324, asdfasdfasdfadsfasdfdsf: 23423432,



}

After:

const a = {
  foo: 123,
  bar: 23,
  baz: 23,
  yes: 1231232132,
  asfasdfasdfasdfas: 342342343243,
  asdfasdfasdfsdfas: 324,
  asdfasdfasdfadsfasdfdsf: 23423432
};

Prettier is really opinionated about the layout, and gives me a nicely formatted object that passes all the eslint rules. They also have lots of heuristics that can keep short objects on the same line when it makes sense. RuboCop and Rufo both leave lines that are way too long, so I have manually fix them.

This is just one (bad) example, but I'm looking for a really opinionated Ruby formatter with lots of heuristics, like Prettier.


There's also the issue of speed. I just tried using RuboCop with formatOnSave in VS Code, and it's unbearably slow, even when just using -x to auto-correct the layout cops.

Rufo is much faster (200ms vs 2.5s), so I would definitely prefer to run Rufo on every save. I think it can stay small and fast because it's just a code formatter. While RuboCop does so many other things so maybe it's better as a linter.

If there's a way to speed up RuboCop and bring down the autofix layout command to under 200ms, then I think it would be a fair comparison.

@bbatsov
Copy link

bbatsov commented Nov 16, 2018

Fair points.

Rufo is much faster (200ms vs 2.5s), so I would definitely prefer to run Rufo on every save. I think it can stay small and fast because it's just a code formatter. While RuboCop does so many other things so maybe it's better as a linter.

Hmm, I didn't know the auto-correct was so slow at this point. Please, open an RuboCop issue so we can investigate what's going on with the performance there. I assume that has to do with the fact that RuboCop incrementally updates the current file (each cop does its changes sequentially) and Rufo generates the code straight from the AST. Likely this is something we can speed up, though.

As for pretty-printing data structures - no one has brought up this so far on RuboCop's side, but we can certainly do something about this. I'd suggest filing a separate ticket about this as well.

@maxh
Copy link

maxh commented Apr 4, 2019

Honestly I'm a bit disappointed that neither one has an opinionated way of splitting up the hash into multiple lines.

FYI I've written a Rubocop cop to split up multiline hashes: rubocop/rubocop#6824

Internally we have an autocorrecting line length cop I'm planning to upstream soon too. In combination, these two cops can pretty print the example you share with Rubocop. Here's the output:

a = {
  foo: 123,
  bar: 23,
  baz: 23,
  yes: 1231232132,
  asfasdfasdfasdfas: 342_342_343_243,
  asdfasdfasdfsdfas: 324,
  asdfasdfasdfadsfasdfdsf: 23423432,

}

That last empty line is an issue but should be easy to fix.

@bbatsov
Copy link

bbatsov commented Apr 4, 2019

@maxh This comment reminded me of your PR and I just merged it! Thanks for working on this.

Btw, on the general topic of the conversation I wrote this article recently.

RuFo, rubyfmt and prettier-ruby seems to have pretty similar goals/scope, so maybe some collaboration there is possible. In particular RuFo and rubyfmt seem to have a lot of overlap. For RuboCop formatting happened accidentally and by the way, so the experience is not as polished as it could be, but with the help of people like @maxh we seem to be making steady progress regardless. Hopefully one day we'll catch up to the rest of the formatters, but RuboCop will always remain committed to being highly configurable (but with very opinionated defaults).

And we'll definitely tackle those performance issues soon. Btw, https://github.com/fohte/rubocop-daemon seems like a pretty decent way to sidestep in the mean time.

@gurgeous
Copy link
Contributor

gurgeous commented Apr 4, 2019

Can confirm - my team is trying out rubocop with rubocop-daemon and it is extremely fast. We also like rufo and want to give prettier-ruby a whirl since we use prettier elsewhere in the project.

@andywatts
Copy link

How did "opinionated defaults" become "remove configuration options".
No longer possible to configure indents to 4 spaces? :/

@martinos
Copy link

martinos commented Jul 23, 2020

@andywatts, if there's a setting that should be fixed it's the spacing. There is almost no gem that uses 4 spaces indentation.

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

No branches or pull requests