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

Clarify usage of "arguments" and "parameters" in error messages #10378

Conversation

HertzDevil
Copy link
Contributor

See #10374.

end

if covered[found_index]
named_arg.raise "argument '#{named_arg.name}' already specified"
named_arg.raise "argument for parameter '#{named_arg.name}' already specified"
Copy link
Member

Choose a reason for hiding this comment

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

Is "for parameter" really helpful to improve this message?

Copy link
Contributor Author

@HertzDevil HertzDevil Feb 19, 2021

Choose a reason for hiding this comment

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

Parameters always have a name but this isn't the case for arguments (in some other error messages they are referenced by numbers like #1 and so on).

Though using "parameter ... already specified" alone is also fine to me.

Copy link
Member

Choose a reason for hiding this comment

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

But this is about a call, so it's a duplicate argument, not a duplicate parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There cannot be duplicate arguments. Positional arguments are always identifiable by unique indices and duplicate named arguments are detected by the parser.

Copy link
Member

Choose a reason for hiding this comment

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

Technically this is correct, yes. But from a user's perspective I would prefer not to be that specific here.
Argument 'foo' already specified is a completely sufficient error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technical correctness is the whole point of this issue. I don't see why this particular error message would be one that users prefer to be less specific than the other messages involving arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Why would shortening argument to argument for parameter be less correct? It's just more concise. Would the shortened form cause any ambiguity or misunderstanding of the error message's meaning?

src/compiler/crystal/types.cr Show resolved Hide resolved
private def check_macro_param_count(a_macro, expected_count)
param_count = a_macro.args.size
if param_count != expected_count
raise TypeException.new "wrong number of parameters for macro '#{a_macro.name}' (given #{param_count}, expected #{expected_count})"
Copy link
Member

Choose a reason for hiding this comment

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

We are being more terse here in case of expected_count <= 1. But it's fine.

@straight-shoota straight-shoota added this to the 1.2.0 milestone Aug 26, 2021
@straight-shoota straight-shoota merged commit dfc79bc into crystal-lang:master Aug 30, 2021
@HertzDevil HertzDevil deleted the refactor/arguments-parameters-errors branch August 31, 2021 05:39
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.

3 participants