[NO-TICKET] RFC: Adopt standardrb for Profiler #3801
Conversation
…he project on rubocop This allows us to experiment with standard without fully committing to it for the full library yet.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
Nothing to fix on this one.
This file is really weird by design + it's only really used for testing so I think it's OK if there's some small inconsistency with the rest of the codebase.
Nothing to fix on this one.
This last set showed up after I adjusted the inline rubocop comments and I missed it.
…`.standard_todo.yml` Thanks for the great suggestion @lloeki; this approach does seem to be a bit cleaner and future-proof.
| @@ -104,7 +104,7 @@ def self.libdatadog_folder_relative_to_ruby_extensions_folders( | |||
| # the Gem.dir. | |||
| expected_ruby_extensions_folders = [ | |||
| "#{gem_dir}/extensions/platform/extension_api_version/datadog_version/", | |||
| "#{gem_dir}/bundler/gems/extensions/platform/extension_api_version/datadog_version/", | |||
| "#{gem_dir}/bundler/gems/extensions/platform/extension_api_version/datadog_version/" | |||
There was a problem hiding this comment.
I think all the cop changes looks super reasonable!
Except for removing the trailing comma from multi-line lists, like this one.
This guarantees that diffs for this list have a useless diff line on the trailing element, just adding a comma.
It's a small thing, but you always makes me slightly confused when I'm reviewing changes and I don't know if someone changed the last element of the list or not. Specially, for long lines, like "#{gem_dir}/bundler/gems/extensions/platform/extension_api_version/datadog_version/", I have to inspect the whole string, instead of it simply never appearing in the diff, because in fact it did not change at all.
There was a problem hiding this comment.
Yeap, I'm with you on that one -- I think trailing comma is the best thing since sliced bread for exactly the reason you pointed out.
But I guess the whole thing with standardrb is to accept it as-is, without customizing so >_>
There was a problem hiding this comment.
accept it as-is
I want to yield such power!
There was a problem hiding this comment.
I see this was already reported: standardrb/standard#611
There was a problem hiding this comment.
Yeah the comma thing is the most annoying one ever (with the "double-quote strings everywhere" as a close second and some occasionally very awkward alignment things like foo = case .. when). I mean beyond diffs, in editors "duplicate line" is a very useful thing.
I really don't understand it, I recall their argument being mostly "well you shouldn't care because it's auto formatted on save".
A workaround is:
- disable the rule(s) at the standard level (standard.yml)
- enforce the chosen rule(s) at the rubocop level
There was a problem hiding this comment.
So it's clear there are ways of bending standardrb to our will on this one.
My question is: do we want to?
I'm leaning towards soft-no just because it goes ~slightly against the spirit of standardrb, but being a soft-no, I'm happy to go in the other direction.
There was a problem hiding this comment.
There's value in not quibbling. See, this PR seems blocked because of this? ;)
We can revisit later if we find it very annoying. Let's just pick something.
| @@ -0,0 +1,3 @@ | |||
| # For available configuration options, see: | |||
| # https://github.com/testdouble/standard | |||
| ruby_version: 2.5 | |||
There was a problem hiding this comment.
| ruby_version: 2.5 | |
| ruby_version: 2.5 | |
| ignore: | |
| - '**/*': | |
| - Style/TrailingCommaInArguments | |
| - Style/TrailingCommaInArrayLiteral | |
| - Style/TrailingCommaInHashLiteral |
I think this is the closest we can get to it 😝
(I'm looking at https://github.com/standardrb/standard/blob/0234926ddf233edfa46479d152dc9aa91171b8fc/config/base.yml#L1765-L1778 for the reference on what to change)
There was a problem hiding this comment.
Interesting... I'd be happy to go with this also, I do like leaving the trailing commas.
I guess let's see what the rest of the team thinks? I'm happy to apply the change, but want to see if we can get consensus :)
There was a problem hiding this comment.
Also, enforce the commas at the Rubocop level.
There was a problem hiding this comment.
It is my opinion that different code fragments call for different formatting in order to most clearly express the intent and minimize confusion and likelihood of errors. A competent programmer can intelligently choose formatting and style on a case-by-case basis to make their code clear to the reader. I find that at least in Ruby, any programmatically enforced code style produces inferior readability and maintainability compared to an intelligent programmer writing their code with care, because any chosen style is worse for some code fragments and the tool demands the selected formatting all the time including when it makes the code harder to read or harder to maintain.
Specifically for this PR, I am a strong believer in trailing commas to reduce diff noise and my biggest issue with the existing rubocop rule set is that it demands inversion of flow in conditionals by demanding the action if condition pattern for single-statement actions, which appears to be retained in this PR (I don't know if standard also demands this inversion of flow or permits remaining inverted conditions to remain, but would also permit normal flow for single-statement actions.)
I think standard is inferior to rubocop due to trailing comma deletion, and they are either tied for conditional flow inversion or possibly standard is better if it does not force flow inversion. Code as I type it does not satisfy any programmatically defined style, therefore whether rubocop or standard is used I would have them reformat everything. As such, the two tools seem to be to be approximately equal. Therefore I do not object to replacing one with the other.
|
Seems like we by and large don't like the comma thing, so I'm OK to globally disable that rule at the standard level.
@p-datadog I agree that intentional formatting helps conveying intent. In practice I've found only a few cases where standard made choices that annoyed me:
Otherwise the remainder is really helpful and not too intrusive, the set of enforced rules is much smaller than Rubocop defaults, and I've been able to format code and convey intent as usual, in ways that Rubocop rule defaults would prevent and be much more annoying. |
|
BTW I chimed in: standardrb/standard#611 (comment) |
|
Thanks everyone! I've excluded the cops that check for trailing commas, so we can continue to use them, and I believe annoyed everyone about this topic so I think we're good to go! I'll press the green button. As usual, this is still open for revisiting, and please do share any issues/concerns :) |
What does this PR do?
This PR replaces the current rubocop style configuration with the standardrb linter for the profiler only.
This PR has several objectives:
And in particular it shows:
.git-blame-ignore-revsto improve blame-ability of updated filesMotivation:
We've been adopting standardrb for our new codebases/gems, so we've recently discussed, and I had an action item to experiment with the profiler adopting standardrb.
Additional Notes:
You'll note that I've added a
.standard_todo.ymlfile rather than fixing every file.Specifically, I've yet to fix the following cops:
This is because fixing each of these cops will create a LOT of diff noise. Thus, I've explicitly not included them in this PR, and plan to fix them separately, in smaller PRs, so we can still sanely review the diff on this PR.
How to test the change?
No behavior changes are expected, so our test coverage should be enough.