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

Fix 'NamedTupleLiteral#[]` to raise an error for invalid key type #7158

Merged
merged 1 commit into from
Dec 7, 2018

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Dec 7, 2018

Ref #7153

And also update doc.

NamedTupleLiteral#[]= denies key typed neither StringLiteral, SymbolLiteral nor MacroId already. I think NamedTuple#[] should do same.

@@ -937,7 +937,7 @@ module Crystal
when StringLiteral
key = key.value
else
return NilLiteral.new
raise "argument to [] must be a symbol or string, not #{key.class_desc}:\n\n#{key}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest avoiding newlines in exception messages if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asterite
Copy link
Member

asterite commented Dec 7, 2018

I don't think this is right. With this, there's no way to know whether a key is present or not in the named tuple literal.

@RX14
Copy link
Contributor

RX14 commented Dec 7, 2018

Guess we need []?

@asterite
Copy link
Member

asterite commented Dec 7, 2018

Maybe. Or just returning nil in macros is fine. It's done in many other places (like in Annotation#[])

@Sija
Copy link
Contributor

Sija commented Dec 7, 2018

AFAIK this exception is raised only when a wrong type of node passed as a key, value can still be nil.

@asterite
Copy link
Member

asterite commented Dec 7, 2018

@Sija oh, right, I looked too quickly. Thanks!

@RX14 RX14 added this to the 0.27.1 milestone Dec 7, 2018
@RX14 RX14 merged commit cbea5ca into crystal-lang:master Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants