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.
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.
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.
That too, but I guess it means this is just a bug in macro splat
There was a problem hiding this comment.
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.
@oprypin Both need to be renamed. Otherwise mapping(properties: String) would still match the other signature.
Sija
force-pushed
the
pr-5180-followup
branch
from
bb71812
to
038f92e
Compare
Sija
force-pushed
the
pr-5180-followup
branch
from
038f92e
to
d0c323a
Compare
|
ping? |
|
Could we get this merged after 2 weeks of silence? |
Followup to #5180.