Add Sorbet/TStructPropertyAttrMacro
#182
Conversation
| context("and the names match") do | ||
| it("registers an offense") do | ||
| expect_offense(<<~RUBY) | ||
| class Foo < T::Struct |
There was a problem hiding this comment.
We should detect this in other classes as well (T::ImmutableStruct, and probably others too).
There was a problem hiding this comment.
I've updated the cop to look for T::ImmutableStruct, and T::InexactStruct as well. I'm not sure if it's worth the trouble to look for anything else (e.g. T::Props), as it would require also scanning for included modules, and would probably be more brittle.
| it("registers an offense") do | ||
| expect_offense(<<~RUBY) | ||
| class Foo < T::Struct | ||
| attr_writer :bar |
There was a problem hiding this comment.
One case in which this might be legit would be if the writer's sig is more restrictive:
class Foo < T::Struct
sig { param(number: Integer) }
attr_writer :number
const :number, Numeric
endLess restrictive would be an error, although would be hard to check for.
This would be pretty unusual though, and arguably a better named method would probably make more sense, so it's probably safe to just assume that if attr_* is being used, it's incorrect usage, and let the author of such an exotic usage simply disable the cop.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
sambostock
force-pushed
the
t-struct-attr-macros
branch
2 times, most recently
from
ee78171 to
f5ed82e
Compare
In a `T::Struct`, `attr_*` methods shouldn't be used for properties. Instead, `prop` should be used instead of `const` to make a mutable property, and all simple accesses should go through the generated methods. The only case where a custom reader or writer should be used is if the logic is being customized.
sambostock
force-pushed
the
t-struct-attr-macros
branch
from
f5ed82e to
1997f2c
Compare
| # end | ||
| class TStructPropertyAttrMacro < Base | ||
| MUTABILITY_MSG = "Use `T::Struct.prop` instead of `%{keyword}` to define `%{name}` property as mutable." | ||
| OVERRIDE_MSG = "Do not override `T::Struct` `%{name}` property %{attr_method_type} unless customizing it." |
There was a problem hiding this comment.
Let's remove the unless customizing it as it's not really recommended and could be confusing here.
| end | ||
|
|
||
| def attr_method_type | ||
| keyword.to_s.delete_prefix("attr_") |
There was a problem hiding this comment.
It may make more sense to actually keep the prefix to make it extra obvious that the attr_writer/attr_reader/attr_accessor is the problem
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
In a
T::Struct,attr_*methods shouldn't be used for properties.