- Notifications
You must be signed in to change notification settings - Fork161
Allow explicit self in STYLEGUIDE.md#228
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon.Learn more
I think this is a case where a consistent style across the codebase reduces cognitive load. I think we either use implicit receiver everywhere, in which case usage of My preference is to maintain the current implicit receiver. That said, I'm bummed that the matching guardrails of detecting shadow variables are currently disabled: rubocop-github/config/default.yml Lines 656 to 663 in5db0a50
Do you think enabling the shadow warnings would address (some) of your concerns? |
If I may I'd push back gently on this assertion:
I feel like you need to inspect it carefully in both the Maybe if we turned on the shadow warnings and therefore could trust that variables arenever shadowed it wouldn't be an issue but I don't think that's something we can guarantee. In terms of
-- I guess enabling the shadow warnings is better than not having them turned on at all? I also totally understand if for whatever reason I am in the minority for this aesthetic preference, the world isn't organized around my pet peeves 😉. |
I worry this is a fairly large change from Ruby-as-generally-written. I do though think we should enable the shadowing linters; that actually seems very problematic that they aren't enabled. I'll work on doing that. |
Hey there!
I just got pinged for@sampart'sPR here where explicit self was being auto-removed from the codebase, and I experienced a negative reaction to it.
As a means of discussion, this PR proposes removing the
avoid explicit use of self
rule – but I don't think I actually care about it as a styleguide preference per se, but I feel like I do disagree with it being auto-enforced.Here is my reasoning:
As the styleguide suggests,sometimes you need to explicitly refer to
self
, i.e. "unless to specify a method shadowed by a variable". Sometime in the distant past, earlier in my career, I got bit by variable shadowing: I updated a local variable instead of referencing a class method, and introduced a bug.Ever since then I have adopted a defensive posture: whenever I am referring to a classes' method, or for example, an ActiveRecord attribute,I always explicitly use
self.foo
. Byalways being explicit, it is impossible to accidentally, later on, in an unrelated change, introduce a bug because a variable got shadowed. In some of these code we work on it's simply impossible for every developer to know every class method.OK Phill, you might say, but why does this matter? Just because you've developed trauma-informed behaviours doesn't mean it's a concern for the rest of us.
Here's the kicker for making it auto-enforced: if you almost always get dinged by the linter, after you've written the code, for using
self.
you will reflexively adopt the opposite stance and avoidself.
as much as possible. This means that when you do actually shadow a variable you might be less likely to pick it up. It imposes a cognitive burden on the developer.1(If I were god-empress of the world, I'd go the full other direction and suggest enforcingalways using explicit self as a means of removing this class of bug altogether, but here I would be satisfied by simply not making MY life more annoying 😉.)
tl;dr:
Thank you kindly for your time & consideration,
Footnotes
An analogy here might be making semi-colons optional in pure Javascript. Yes, it works, except in occasions where it introduces a bug. So to support removing semi-colons, you need to be perfectly aware of the edge cases where it introduces a bug. That sounds a lot harder than just using semi-colons!↩