Skip to content
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

JSON: Change properties key to _properties_ to reduce chance of conflict #5180

Merged
merged 1 commit into from
Oct 26, 2017

Conversation

maxpowa
Copy link
Contributor

@maxpowa maxpowa commented Oct 24, 2017

Relates to #4595, _properties_ is less likely to conflict than properties is.

@maxpowa maxpowa changed the title Change properties key to _properties_ to reduce chance of conflict JSON: Change properties key to _properties_ to reduce chance of conflict Oct 24, 2017
macro mapping(properties, strict = false)
{% for key, value in properties %}
{% properties[key] = {type: value} unless value.is_a?(HashLiteral) || value.is_a?(NamedTupleLiteral) %}
macro mapping(_properties_, strict = false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does %properties work for macro arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, syntax error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't make sense, because the macro argument properties is a macro-level variable, whereas %-prefixed variables are local variables in the generated code. This are two different levels.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it obviously doesn't work, but it is unfortunate that it is impossible to avoid collisions.

@luislavena
Copy link
Contributor

Thanks for your contribution @maxpowa!!!

It seems that CI failed due differences in the format.

Can you make sure crystal tool format is run on the code and push again?

For more details, please see the documentation about coding styles

Thank you!
❤️ ❤️ ❤️

@maxpowa maxpowa force-pushed the change-json-named-args branch from 07d8645 to e4beb7d Compare October 24, 2017 21:07
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! But we still have this problem with the strict property, though I don't know how to solve that. In any case, one can pass a Hash or NamedTuple to avoid clashing with properties or strict, so this change is not strictly necessary, but still welcome.

@straight-shoota
Copy link
Member

straight-shoota commented Oct 25, 2017

@asterite I don't think strict is really an issue. In order to use this, you need to provide the properties as a NamedTuple/Hash literal anyway. It can't be used as a positional argument because JSON.mapping(title: String, true) is invalid - and JSON.mapping(title: String, strict: true) is interpreted as one NamedTupleLiteral.

It would perhaps be possible to introduce a special check if _properties_ is a NamedTupleLiteral and it's last key is strict with a BoolLiteral value. But that would make the code much more complicated with IMHO little improvement to be able to use JSON.mapping(title: String, strict: true) instead of JSON.mapping({title: String}, strict: true).

@RX14 RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib labels Oct 26, 2017
@RX14 RX14 added this to the Next milestone Oct 26, 2017
@RX14 RX14 merged commit 4d587a3 into crystal-lang:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants