-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Change properties key in YAML.mapping to reduce chance of conflict #5352
Conversation
@@ -216,7 +216,7 @@ module YAML | |||
|
|||
# This is a convenience method to allow invoking `YAML.mapping` | |||
# with named arguments instead of with a hash/named-tuple literal. | |||
macro mapping(**properties) | |||
::YAML.mapping({{properties}}) | |||
macro mapping(**_properties_) |
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'm pretty sure that changing these 2 lines is all that's needed. No idea why the other PR went that far.
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 don't think you need to change this macro, the splat arg name cannot be used in a call IIRC, so cannot clash
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.
That too, but I guess it means this is just a bug in macro splat
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.
Nevermind, forget what I said, I didn't know one could do https://carc.in/#/r/37af
I thought this would give:
{props: {a: 1}}
instead (maybe that's what it should do?)
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.
@oprypin Both need to be renamed. Otherwise mapping(properties: String)
would still match the other signature.
bb71812
to
038f92e
Compare
038f92e
to
d0c323a
Compare
ping? |
Could we get this merged after 2 weeks of silence? |
Followup to #5180.