Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
phillmv wants to merge1 commit intomainfromphillmv/allow-explicit-self

Conversation

phillmv
Copy link
Member

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 theavoid 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 toself, 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 useself.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 usingself. 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:

  • enforcing implicit self does not catch bugs or improve performance
  • enforcing implicit self can, occasionally, introduce bugs
  • i reflexively always use explicit self and this would impose a burden on me personally, which we can all agree is a great tragedy

Thank you kindly for your time & consideration,

Footnotes

  1. 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!

sampart reacted with eyes emoji
@CopilotCopilotAI review requested due to automatic review settingsJanuary 10, 2025 18:01
@phillmvphillmv requested a review froma team as acode ownerJanuary 10, 2025 18:01
Copy link

@CopilotCopilotAI left a 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

@bensheldon
Copy link
Contributor

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 ofself implies a shadowing. Or we use an explicitself everywhere, in which case it's absence implies a local variable. Mixing them in the same codebase is means that everything must be inspected more deeply.

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:

Lint/ShadowedArgument:
Enabled:false
Lint/ShadowedException:
Enabled:false
Lint/ShadowingOuterLocalVariable:
Enabled:false

Do you think enabling the shadow warnings would address (some) of your concerns?

@phillmv
Copy link
MemberAuthor

If I may I'd push back gently on this assertion:

I think this is a case where a consistent style across the codebase reduces cognitive load. … Mixing them in the same codebase is means that everything must be inspected more deeply.

I feel like you need to inspect it carefully in both thedo_nothing AND theenforce_implicit options?

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 ofcognitive_load(option), I would sort them like this?

cognitive_load(enforce_implicit) > cognitive_load(do_nothing) > cognitive_load(enforce_explicit)

--

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 😉.

@bensheldon
Copy link
Contributor

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.

sampart reacted with heart emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@phillmv@bensheldon

[8]ページ先頭

©2009-2025 Movatter.jp