-
Notifications
You must be signed in to change notification settings - Fork 208
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
parser: enable record types with optional fields #1717
Conversation
@soutaro this is a draft proposal, as in, the optional fields are parsed and stored, but not used yet. If you agree with this, I can proceed and use it in inferences (may need some help with that eventually as well). |
72cc2cf
to
d077832
Compare
@HoneyryderChuck Thanks! Let's go with the syntax. Having different hash tables for required and optional fields would result breaking the order of the records, for class Record
attr_reader fields: Hash[Symbol, [t, bool]]
type loc = Location[bot, bot]
def initialize: (fields: Hash[Symbol, t], location: loc?) -> void # For compatibility with 3.4
| (all_fields: Hash[Symbol, [t, bool]], location: loc?) -> void
include _TypeBase
def fields: () -> Hash[Symbol, t]
def optional_fields: () -> Hash[Symbol, t]?
end |
864611b
to
451fd78
Compare
@soutaro can you check the changes? I believe this is what you were aiming for? |
451fd78
to
cdde385
Compare
Thanks @HoneyryderChuck! Looks great. Do you think it's ready to merge? |
|
||
include _TypeBase | ||
|
||
attr_reader location: loc? | ||
|
||
def fields: () -> Hash[Symbol, t] | ||
|
||
def optional_fields: () -> Hash[Symbol, t]? |
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.
Ah, I made a mistake to put ?
at the end of the method type. It should return an empty hash.
def optional_fields: () -> Hash[Symbol, t]? | |
def optional_fields: () -> Hash[Symbol, t] |
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.
if you look at the implementation, nothing is returned if all_fields is the same as fields. I think that's fine, i.e. we can avoid creating that extra hash when not necessary. Or do you see any use for it?
cdde385
to
8e34843
Compare
this enables richer definitions of record types, which can only be defined nowadays with Hash[Symbol, untyped].
8e34843
to
8f506ea
Compare
It depends: the optional field set is not yet being used in inferences. Do you think that's a separate change? If so, I'd say yes, it's ready to merge. Separate change or not, will you do it, or shall I perhaps try it out? (pointers on what/where definitely welcome if so 🙏 ) |
@soutaro anything on the above? |
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.
Thanks! 👍
this enables richer definitions of record types, which can only be defined nowadays with Hash[Symbol, untyped].
To handle #504