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 error message for calling record macro with kwargs #13367

Merged
merged 7 commits into from
May 12, 2023
Merged

Fix error message for calling record macro with kwargs #13367

merged 7 commits into from
May 12, 2023

Conversation

a-alhusaini
Copy link
Contributor

Thanks to straight-shoota, hertdevil, blacksmoke, and devonte's thoughts and ideas we now get this error message when a named arg is provided to record

Error: Cat does not accept named arguments

    You probably wrote:

      record Cat, name: String

    instead of:

      record Cat, name : String

The error is a bit long but I believe it is better than just telling the user that record does not accept named arguments. The space costs are more than made up for by the clarity of the message.

src/macros.cr Outdated Show resolved Hide resolved
src/macros.cr Outdated Show resolved Hide resolved
…of writing the name. Clarified error message.
src/macros.cr Outdated
Comment on lines 68 to 69
record #{name}, #{(properties + kwargs.map{|name, value| "#{name} : #{value}"}).join(", ").id}
" unless kwargs.empty? %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The trailing whitespace will still show up:

Suggested change
record #{name}, #{(properties + kwargs.map{|name, value| "#{name} : #{value}"}).join(", ").id}
" unless kwargs.empty? %}
record #{name}, #{(properties + kwargs.map { |name, value| "#{name} : #{value}" }).join(", ").id}" unless kwargs.empty? %}

@HertzDevil
Copy link
Contributor

This is not unique to this PR, but I wonder why the compiler inserts a newline right after the first line of the error message itself

src/macros.cr Outdated Show resolved Hide resolved
@a-alhusaini
Copy link
Contributor Author

This current solution does not work with Strings. I am not sure why.
I tried
record Cat, name: String
This shows undefined constant
record Cat, name: Int64
This shows the error we want.

Any ideas?

@Blacksmoke16
Copy link
Member

@a-alhusaini FWIW both of those examples produce an undefined constant error. I imagine it's just because name an actual parameter on the record macro so it's not actually being captured in kwargs.

@HertzDevil
Copy link
Contributor

Such an argument mismatch will always be possible as long as there is a positional parameter for the record's name, and Crystal does not have purely positional parameters. There are 2 possible solutions: (the second is probably better here)

# uglier, but ensures no mismatch can ever happen
macro record(*name_and_properties, **kwargs)
  {% name = name_and_properties[0] %}
  {% properties = name_and_properties[1...name_and_properties.size] %}
end

# names containing `__` are reserved by Crystal, so technically
# no user code should ever use a named argument for `__name`
macro record(__name name, *properties, **kwargs)
end

src/macros.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
@straight-shoota straight-shoota added this to the 1.9.0 milestone May 11, 2023
@straight-shoota straight-shoota changed the title record now accepts kwargs and raises if they are povided Fix error message for calling record macro with kwargs May 12, 2023
@straight-shoota straight-shoota merged commit 1a43461 into crystal-lang:master May 12, 2023
This pull request was closed.
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.

Undefined constant with record and improper properties
5 participants