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

B026 - Argument unpacking after keyword argument #287

Merged
merged 4 commits into from Sep 22, 2022
Merged

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Sep 20, 2022

Closes #286.

bugbear.py Outdated Show resolved Hide resolved
bugbear.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

All looks good to me. I would love a second review from an AST stronger person than I am. Maybe adding my test suggestion might cover another edge case we should support but I am not sure ...

(yes it's ironic that someone so AST challenged maintains this project)

tests/b026.py Outdated Show resolved Hide resolved
@pmeier
Copy link
Contributor Author

pmeier commented Sep 20, 2022

I would love a second review from an AST stronger person than I am

Any suggestions?

@Zac-HD
Copy link
Member

Zac-HD commented Sep 20, 2022

This looks pretty good to me, with the substantial caveat that function calls can have multiple unpackings and you're only checking the first:

def f(a, b, c, d):
    print(a, b, c, d)

f(
    *[1],  # currently no lint error, because this is before the keyword and we stop at the first
    d=0,
    *[2],  # want a lint here
    *[3],  # and also here
)
# prints: 1 2 3 0

@cooperlees I assume you know about greentreesnakes docs? They make me look better at AST stuff than I deserve 😉

@pmeier
Copy link
Contributor Author

pmeier commented Sep 21, 2022

substantial caveat that function calls can have multiple unpackings and you're only checking the firs

Good catch!

@pmeier pmeier requested review from cooperlees and Zac-HD and removed request for ambv, Zac-HD and cooperlees September 21, 2022 08:04
@pmeier
Copy link
Contributor Author

pmeier commented Sep 21, 2022

Sorry for the weird review requests. GH doesn't let me do it properly.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 21, 2022

Implementation looks great to me - thanks!

Before merging I'd also want a longer/more informative message, along the lines of 'To avoid confusing or misleading calls, put star-args before keyword arguments so that the order of arguments matches the order of the parameters.'. Not delighted by that wording, but I think it's important to say why as well as what to help the user learn. And better to comment now than come back with something better a week late 😅

README.rst Outdated
@@ -160,6 +160,8 @@ the loop, because `late-binding closures are a classic gotcha
This check identifies exception types that are specified in multiple ``except``
clauses. The first specification is the only one ever considered, so all others can be removed.

**B026**: Argument unpacking after keyword argument.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**B026**: Argument unpacking after keyword argument.
**B026**: Argument unpacking after keyword argument is strongly discouraged. This behavior is uncommon and can lead to unnecessary confusion when reading the code thus making it prone to causing bugs. There was cpython discussion of disallowing but legacy usage and parser limitations make it difficult.

+1 on more description here. A suggestion that I'm more than happy to have reworded + made better.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**B026**: Argument unpacking after keyword argument.
**B026**: Star-arg unpacking after a keyword argument is strongly discouraged, because
it only works when the keyword parameter is declared after all parameters supplied by
the unpacked sequence, and this change of ordering can surprise and mislead readers.

I think this is compact enough to also use as the runtime message, and explains specifically what we think is wrong (the mismatched order can be misleading) rather than non-actionable historical details.

I do like the historical details and I'd be happy to keep "There was cpython discussion of disallowing this syntax, but legacy usage and parser limitations make it difficult." with that link in the readme too, but not print it at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super happy about the wording and needed to read it multiple times to understand that it indeed correctly explains the issue. Still, I have no better way so what gives. This is really hard to explain without an example TBH.

+1 on keeping the historical details to the README.

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Awesome. All LGTM.

@cooperlees cooperlees merged commit 2b4c163 into PyCQA:main Sep 22, 2022
6 checks passed
@cooperlees
Copy link
Collaborator

Will work on #288 and release after we merge that.

@pmeier pmeier deleted the nuakw branch September 22, 2022 19:22
Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Oct 5, 2022
For better or worse this is already part of this function's API.
clrpackages pushed a commit to clearlinux-pkgs/pypi-jsonschema that referenced this pull request Dec 28, 2022
…version 4.17.3

Julian Berman (28):
      Make it clearer that format: regex is Python regexes in this implementation.
      Better wording.
      Bold is loud.
      Ignore PyCQA/flake8-bugbear#287.
      Fix the grammar on ErrorTree's repr when it has 1 error.
      Deprecate the CLI (via import or running normally).
      Update the Draft 3 and 4 metaschemas.
      Enable format validation by default in check_schema.
      Re-add the second test for importing jsonschema.cli
      Satisfy mypy's new default.
      Squashed 'json/' changes from ed0b855e7..0fa89d2ab
      Squashed 'json/' changes from 0fa89d2ab..9251ebff1
      Emit a better error message for unevaluatedProperties with a subschema.
      Un-bundle single-vocabulary meta-schemas.
      Update docs requirements.
      Temporarily evade wpilibsuite/sphinxext-opengraph#87
      Suppress epub warnings for duplicated ToC entries.
      Run more things with 3.11 in CI.
      Skip the rest of the docs builds on Windows in CI.
      Remove making believe we are ReadTheDocs in CI builds.
      v4.17.1 -> CHANGELOG
      Squashed 'json/' changes from 9251ebff1..78c888273
      Minor fix for test case class names (for format tests).
      Empty strings are not valid relative JSON pointers.
      Durations without trailing units aren't valid durations.
      Update pre-commit hooks.
      Try fixing more Sphinx refs which fail only on Ubuntu...
      Fix instantiating validators with cached refs-to-bool schemas.
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.

Proposed check: don't allow argument unpacking in a call after a keyword argument
3 participants