-
-
Notifications
You must be signed in to change notification settings - Fork 10k
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
refactor: inline use of attr_predicate #19371
base: master
Are you sure you want to change the base?
Conversation
Hmm brew/Library/Homebrew/dev-cmd/audit.rb Lines 251 to 261 in cdb6a3d
@foo == true or !!@foo instead of the simpler @foo for the inlined methods. Sadness.
|
Found the rationale for this distinction: #12375 (comment) |
3a81bdf
to
ebdd11f
Compare
ebdd11f
to
315c0c2
Compare
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.
I went into this skeptical but: yeh, I prefer this, particularly now we have some Sorbet guarantees. Nice work @dduugg!
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?The follow-up to #19359 which removes remaining use of the
Attrable
module. This may be less desirable, as it's used more extensively (n = ~50) and it already had correctly compiledsig
s.The defined method for an
attr_predicate :foo
was@foo == true
, but I was able to reduce this to being provably equivalent to just returning@foo
in all but two cases (both involving metaprogramming inFormula
, which would require a larger refactor to fix). This was possible due to additional refactoring that replacednil
passing withfalse
values instead, but tests seem to indicate that was a valid change.Using endless
def
s for these simple methods make this a net removal of >100 LoC, even with the introduction newsig
s, so that hopefully atones for the loss of conciseness ofattr_predicate
. (There is additional upside in not requiring new code readers to learn this brew-specific abstraction, IMO.)With apologies for the scope creep in this PR, I was also able to promote a few files to
typed: strict
in this change set. I'm happy to decompose into separate PRs at a reviewer's request.