RFC: new Attribute#ivar API
#2110
Conversation
| @@ -304,6 +304,21 @@ def update(name: self.name, type: self.type, ivar_name: self.ivar_name, kind: se | |||
| visibility: visibility | |||
| ) | |||
| 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.
| @@ -185,6 +185,8 @@ module RBS | |||
| include _HashEqual | |||
|
|
|||
| 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:
|
My proposal for fixing #2109. Feedback welcome!