Conversation
| ) | ||
| end | ||
|
|
||
| def ivar |
There was a problem hiding this comment.
Now the semantics are clear: You either get back an InstanceVariable object, or nil (meaning this attribute was declared with no backing ivar declaration, e.g. attr_reader foo(): Foo).
Open question: How should we model the difference between these two?
attr_reader a: Foo # Ivar name is inferred
attr_reader a(@a): Foo # Same ivar name, but explicit
There was a problem hiding this comment.
No difference between the two looks fine, if we consider this is a kind of utility class.
If we really need the detail of the syntax, we can use the old API.
|
|
||
| def update: (?name: Symbol, ?type: Types::t, ?ivar_name: Symbol | false | nil, ?kind: kind, ?annotations: Array[Annotation], ?location: loc?, ?comment: Comment?, ?visibility: visibility?) -> instance | ||
|
|
||
| def ivar: () -> InstanceVariable? |
There was a problem hiding this comment.
Simple return type! :)
There was a problem hiding this comment.
Does it still make sense for ivar_name to accept false in the constructor and the attr_reader?
There was a problem hiding this comment.
Good question, since there's backwards compatibility concerns there.
We can add a new keyword arg (perhaps explicit_ivar_name:?) and use that, while still supporting the old.
amomchilov
force-pushed
the
new-attr-ivar-api
branch
from
1fff94e to
e406183
Compare
| self.ivar_name # Use the custom instance variable name given by the user | ||
| end | ||
|
|
||
| InstanceVariable.new(name: ivar_name, type: type, location: location, comment: comment) |
There was a problem hiding this comment.
Open question: what should the location and comment be? The same as the attribute, or nil?
There was a problem hiding this comment.
I don't think we should have the comment attribute, because it's given to the attribute syntax, not to the instance variable name.
Not very sure about location, but I think going without location should work for now.
There was a problem hiding this comment.
We still have to fill in the parameters, are you suggesting I pass location: nil, comment: nil?
There was a problem hiding this comment.
Oops, I'm sorry. I was confused.
I thought the InstanceVariable is a new class for this API, but actually it is an existing class which models instance variable declaration.
|
I'm generally good for this. My suggestion is dropping |
|
I'm sorry for confusion. 🙇♂️ I think reusing the AST structure is good for this, while introducing a new class looks a bit too much. Simply adding a new methods seem the best way:
|
3 participants
My proposal for fixing #2109. Feedback welcome!