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

Proposal to reconsider trailing dot position #75

Closed
JoshCheek opened this issue Feb 5, 2019 · 18 comments
Closed

Proposal to reconsider trailing dot position #75

JoshCheek opened this issue Feb 5, 2019 · 18 comments

Comments

@JoshCheek
Copy link
Contributor

JoshCheek commented Feb 5, 2019

The setting we want to consider

So we have this config setting.

Layout/DotPosition:
  Enabled: true
  EnforcedStyle: trailing

I'd like to make some arguments against it, provide counterarguments to the reason it's the default, and then wrap up with a summary.

Arguments against it

It causes code churn in git

@samphippen mentioned in the podcast that they choose syntactic preferences with the goal of reducing git churn. Lets analyze this setting with that metric in mind:

We append a line and look at the diff. What we see is that a leading dot causes a 1-line diff and a trailing dot causes a 3-line diff because we had to modify the end of the previous line. Not a huge deal, but it does increase cognitive overhead. You just have to be more on top of your game because the diffs have a less favourable signal to noise ratio.

diff

It's cumbersome to edit

Lets swap the last 2 lines instead. We find we can't just move lines around, because each line is not self-contained, it's partially on the line before it. Since the last line isn't before anything, it deviates from the pattern and we have to futz around with its trailing dot. The last line perpetually needs our attention.

sib-alternative-4

It puts important info way off at EOL

How one line relates to the next is pretty important, but there's lots of reasons a line might continue,
in this example, maybe it's a method call, or maybe it's line1 &&\n line2 or line1 -\n line2 and so on. Important info is hard to access making this difficult to read.

screen shot 2019-02-05 at 1 59 35 am

Addressing the incumbent reasoning

Why is it set this way?

The readme gives the reasoning (here):

Trailing dots on multi-line method chains - chosen because it makes copying lines into a REPL easier

This is a pragmatic solution, and it does accomplish the stated goal, but if buggy REPLs are this problematic, then shouldn't we fix them instead?

Alternative: fix the REPLs

pry-fix-5

There's more to it than my exploration addressed, but that hopefully illustrates that it's within our ability to do. We don't need every codebase to adopt disadvantaged syntax to deal with REPL bugs. We can instead fix the REPL bugs. I've reported my findings here.

Alternative: jump into an editor

In pry, you can also use edit if the thing you're building is getting unwieldy. Then you can work with it in your text editor (similar to bash's C-x C-e).

pry-fix-6

Alternative: start the REPL from the code of interest

But honestly, the desire to paste code into REPLs isn't very compelling. The REPL is line based, so it's not a great editor (plus, Readline is just not very good at it). Instead, of trying to bring the editor into the REPL, use the editor to put the REPL in the context of interest. This is super powerful because you can jump into any context you can invoke (I often set up a test to reproduce getting into that environment, and then put a pry at the confusing spot so I can poke around).

sib-alternative-5

Alternative: Seeing is Believing

Oh, and I'll pitch my gem, too: REPLs are useful for very specific types of problems. Despite the didactic interestingness of the Little LISPer, I wouldn't do these kinds of experiments in a REPL, I'd use SiB.

sib-alternative-3

Yeah... it can handle leading dots like it's nobody's business!

Conclusion

To summarize my points:

  • It generates distracting diff churn
  • It is cumbersome to edit
  • It hides adjacent lines' relationship way off at the end of the line
  • The stated reason is that it's to compensate for REPL bugs, but we can fix the REPLs
  • There are numerous alternatives to pasting code into the REPL, many of which are likely better.

For these reasons, I propose that we change the setting from trailing to leading.

Cheers.

@jasonkarns
Copy link
Collaborator

jasonkarns commented Feb 5, 2019

Aesthetically, and for all the other reasons listed, I agree with @JoshCheek . However, there is one single reason against leading and, IMO, it trumps all the others: single-line comments mid-chain:

foo.
  bar.
  # baz.
  boo

No problems. 😎

foo
  .bar
  # .baz
  .boo

syntax error, unexpected '.', expecting end-of-input. 😢

It might just be a personal thing, but it seems I'm frequently commenting out sections in a chain when debugging; but the leading dot prevents that capability.

@deanrad
Copy link

deanrad commented Feb 5, 2019

That's an unfortunate Ruby parsing error.. In JavaScript, SLCMC (Single Line Comments in the Middle of the Chain) works, and PrettierJS formats with leading dots. Perhaps SLCMC could be the mentioned reason in the Readme @jasonkarns - I know many Rubyists dont even use a REPL (I know, right?!)

The one of @JoshCheek points I most resonate with is that of context, because with the autoloader, ruby/rails in general, it can be hard to see where method names are resolved.

@JoshCheek
Copy link
Contributor Author

JoshCheek commented Feb 6, 2019

foo
  .bar
  # .baz
  .boo

syntax error, unexpected '.', expecting end-of-input. 😢

It might just be a personal thing, but it seems I'm frequently commenting out sections in a chain when debugging; but the leading dot prevents that capability.

Yeah, that's super frustrating 😞 Let me be serious for a minute, but then I'll come back to this, it's going to super make your day!

Alright, the serious bit:

Okay so, as with the REPL issue, it's a bug and I'd prefer that we fix bugs.

It can still be fine to make a tradeoff like this, if it was really one-sided, but the frequency that I chain a method call on the next line far exceeds the frequency that I need to comment one of them out. In fact, if I didn't chain the call, there wouldn't be anything to comment out, so chained calls are a superset of commented out chained calls.

IDK, maybe my predisposition to act on the assumption that bugs should be fixed is less effective in practice than working around them. But even if so, we're not making that decision for ourselves, we're making it for everyone abiding by the standard. For many devs, that will be compulsory, removing their freedom to use their own judgement. This leaves us in an ethically precarious position. If we get it wrong, then not only are we imposing our will onto them, but it will be harmful, for them as well. And once a choice is made, it will be difficult to unmake (a 50k line commit is a tough pill to swallow due to its havoc on VC history) so it's imperative that we are careful and rigorous. If we aren't, we become RuboCop, who presumably doesn't fix the problems @searls pointed out due to this very reason. So new people pick it up, impose its poor configuration on their team, and everyone suffers. We were actually not allowed to fix the config lest it get out of sync across repositories. But, we have to be even more careful, because unlike RuboCop, standard disallows configurability. Given its name, this makes sense, but leaves little recourse for users.

And I mean, at the end of the day, I don't care very much if the dots are at the end or the beginning. One is tedious, but so is lots of code. It's more just that we can't be cavalier about this. Even the little things matter, because if it becomes pervasive, then every cost and every benefit get multiplied. So, if we're going to be choosing a standard, then yo, lets choose metric, not imperial.

Anyway, bringing it all back around now: for something like a bug, its existence is unintentional and thus, likely ephemeral. But once a choice like this becomes a standard, it spreads throughout a codebase and will likely be propagated by dev-culture and habit far beyond the domain of the standard. Generations after the bugs are fixed, devs will be inheriting trailing dots the way we inherit browser workarounds for ie6 and interview questions from 70s computer science curricula.

Okay, now the fun bit:

It might just be a personal thing, but it seems I'm frequently commenting out sections in a chain when debugging; but the leading dot prevents that capability.

I furrowed my brow yesterday and dug into this. Took some pacing and head scratching, and a handfull of failures, but in the end, I was able to fix it! It's on this branch, here, I'll submit a PR for it maybeh tomorrow or if tomorrow is too busy, maybe the day after. Based on a comment in one of the bugs.rubylang issues about it, I made it work for whitespace, as well. But as I'm looking at it, I'm not sure whether that's a good call or not. I'll solicit some feedback on that when I submit it. Anyway, here's a demo ^_^

comment-chain-breaker-bug4

@codenamev
Copy link
Contributor

Another advantage of leading dots is it makes it easier to search for method invocations. I'd be interested in others' workarounds to this.

@majkelcc
Copy link

majkelcc commented Feb 7, 2019

Another big one in favor of leading dots is that it makes it easier to spot errors like this missing dot:

str
  .chars
  .reverse
  drop
  .each_slice
  .to_a

str.
  chars.
  reverse
  drop.
  each_slice.
  to_a

@JoshCheek
Copy link
Contributor Author

Another advantage of leading dots is it makes it easier to search for method invocations. I'd be interested in others' workarounds to this.

I use ag and in this situation I use the -w flag, which basically turns /word/ into /\bword\b/. Sometimes I have to get a little more creative with its options. In particular, -a, --ignore-dir, -s, and I don't see it in the man page, but there's some dynamic options around languages, eg you can do --ruby to only search Ruby files (you can discover these with ag --list-file-types)

image

@strzibny
Copy link

strzibny commented Feb 7, 2019

I agree 100% with this change. The git churn should be taken seriously here.

@searls
Copy link
Contributor

searls commented Feb 7, 2019

Ok, it sounds like support is pretty overwhelmingly in favor of this change.

@JoshCheek would you like to do the honors of sending a PR to update the rules and README so your git blame can live on forever for posterity?

@JoshCheek
Copy link
Contributor Author

@JoshCheek would you like to do the honors of sending a PR to update the rules and README so your git blame can live on forever for posterity?

Submitted ^_^

I'll get that Ruby PR in soon, too, mostly waiting to see if anyone you pinged reaches out. If not, I'll get it figured out, it'll just take a little research.

@trptcolin
Copy link

@JoshCheek I like the reasoning you’ve laid out (which has changed my preference), and I’m sympathetic to your view of fixing bugs rather than working around in general. But I did want to raise the caveat that those bugs will remain for folks on older versions than [FUTURE].

So if StandardRB consumers are on currently-existing versions of Ruby & REPLs, they will need to deal with the downsides others have listed. Maybe not a blocker, but I’d expect even for a new tool like this, backwards-compatibility may be worth considering.

In particular: if I have a chain with trailing dots, with an intermediate line commented out, am I understanding correctly that this proposal mean that reformatting will break working code in existing Ruby versions? I do tend to take a pretty hard view that a code formatter should never break working code, but perhaps there’s a mechanism to pin to a version where the parser bug (that @jasonkarns points out) is fixed?

JoshCheek added a commit to JoshCheek/standard that referenced this issue Feb 8, 2019
@JoshCheek
Copy link
Contributor Author

JoshCheek commented Feb 8, 2019

@JoshCheek I like the reasoning you’ve laid out (which has changed my preference), and I’m sympathetic to your view of fixing bugs rather than working around in general. But I did want to raise the caveat that those bugs will remain for folks on older versions than [FUTURE].

So if StandardRB consumers are on currently-existing versions of Ruby & REPLs, they will need to deal with the downsides others have listed. Maybe not a blocker, but I’d expect even for a new tool like this, backwards-compatibility may be worth considering.

Yeah, that's a good point. I haven't explored the tool very deeply, but I have noticed that there are separate settings files for different versions: https://github.com/testdouble/standard/tree/master/config

image

I interpret this to mean that an older Ruby which did not have such a setting could be customized accordingly.

However, if the circumstances were less favourable, then I'd prefer if we either left it unspecified, allowing the user to choose what was appropriate for them, or made the choice that would age well (over time, the total burden should trend towards negligible, so if someone must be imposed upon, then it's better to impose upon the older versions than the newer ones)

In particular: if I have a chain with trailing dots, with an intermediate line commented out, am I understanding correctly that this proposal mean that reformatting will break working code in existing Ruby versions? I do tend to take a pretty hard view that a code formatter should never break working code, but perhaps there’s a mechanism to pin to a version where the parser bug (that @jasonkarns points out) is fixed?

I would consider it a bug if it did this. RuboCop does have a setting for you to declare your version, I assume the purpose for this is that it should constrain its settings to only the versions they apply to. However, in practice, I've had it fail my code, telling me to use features that were not available to the version I was using. I'm not an expert on RuboCop, but I think we should consider any such occurrences as unexpected.

@kurko
Copy link

kurko commented Feb 8, 2019

@trptcolin I think we need some leeway here in terms of breaking changes because of the following from the README:

[NOTE: until StandardRB hits 1.0.0, we consider this configuration to be a non-final work in progress and we encourage you to submit your opinions (and reasoned arguments) for the addition, removal, or change to a rule by opening an issue. If you start using Standard, don't be shocked if things change a bit!]

I'd suggest that given we're pre 1.0.0, locking your projects to 0.0.26 (as of today) would be the best approach to avoid breaking stuff, as opposed to releasing this change only for the next Ruby version. Thoughts?

@searls
Copy link
Contributor

searls commented Feb 8, 2019

Honestly, I'd be surprised if there is a wide userbase of standard early adopters who would simultaneously have their feathers ruffled by a release that broke these very rare cases (for all I know, haven't checked, the Rubocop autocorrector for this cop might actually bail out in the case of a commented line). I think to @kurko's point, it'd be fine to just patch release and mea culpa.

@JoshCheek if you look at those Ruby 1.8/1.9 YAML configs, they're really there for significant changes of Ruby language features (for example, now kwargs below 2, no symbols hash literal entries before 1.9), and my hope is to keep them super minimal and focused on that. Something as high-churn as which side of the screen every . goes would be surprising/jarring for anyone upgrading a point release of rubby

@bobmaerten
Copy link
Contributor

bobmaerten commented Feb 11, 2019

I'd like to thank @JoshCheek so much for all this work. Change is not an easy thing, but when it comes with realistic and pragmatic explanations either on the "why" and the "how" sides, it feels much easier to embrace.

@jfahrer
Copy link

jfahrer commented Apr 2, 2019

@JoshCheek did you ever get the Ruby PR out?

@ccutrer
Copy link

ccutrer commented Apr 12, 2021

Note that as of IRB 1.3.5, multi-line pastes with leading dots are properly processed (see ruby/irb#202).

@searls
Copy link
Contributor

searls commented Apr 12, 2021

That's awesome, thank you for letting us know and for contributing to Ruby, @ccutrer!!

@aycabta
Copy link

aycabta commented Apr 16, 2021

😎

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