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

Remove 'too many arguments' error when interpolating over mappings #520

Conversation

TheSignPainter98
Copy link
Contributor

When no conversions are specified, interpolation over an empty tuple is accepted, but interpolation over an empty dict is rejected:

>>> 'foo' % ()
"foo"
>>> 'foo' % {}
Traceback (most recent call last):
  <stdin>:1:7: in <expr>
Error: too many arguments for format string

For context, extra dict arguments are not rejected when a conversion is present:

>>> '%(a)d' % {'a': 1, 'unused': 2}
"1"

This PR relaxes the 'too many arguments' check for mappings and adds tests to ensure consistency between tuples and dicts

@TheSignPainter98 TheSignPainter98 changed the title Remove incorrect 'too many arguments' error when interpolating over mappings Remove 'too many arguments' error when interpolating over mappings Nov 20, 2023
Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Code change looks sound.

Could you also add this line to the doc/spec.md file? Thanks.

After this paragraph:

If the "%" is immediately followed by "(key)", ...

add a new paragraph:

If the format string contains no conversions, the operand must be an empty tuple.

Thanks.

starlark/eval.go Outdated Show resolved Hide resolved
@TheSignPainter98
Copy link
Contributor Author

Adding the line to the spec, I think the suggestion isn't quite right as an empty mapping would also be acceptable. Perhaps something like this would work?

If the format string contains no conversions, the operand must be a Mapping or an empty tuple.

@TheSignPainter98 TheSignPainter98 force-pushed the empty-container-string-interpolation-consistency-fix branch from c5a2d5e to 0c90a3d Compare November 21, 2023 13:55
Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

Thanks again!

starlark/library.go Show resolved Hide resolved
@adonovan adonovan merged commit 90ade8b into google:master Nov 21, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants