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

Require named arguments to fix pylint too-many-positional-arguments #273

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

clelange
Copy link
Collaborator

@clelange clelange commented Oct 4, 2024

Fixes #272


📚 Documentation preview 📚: https://hepdata-lib--273.org.readthedocs.build/en/273/

@clelange clelange changed the title Require named arguments to fix pylint too-many-positional-arguments Draft: Require named arguments to fix pylint too-many-positional-arguments Oct 4, 2024
@clelange
Copy link
Collaborator Author

clelange commented Oct 4, 2024

This needs some more fixes in other places, I had only addressed the errors that showed up first in the tests.

@clelange clelange marked this pull request as draft October 4, 2024 16:10
@clelange clelange changed the title Draft: Require named arguments to fix pylint too-many-positional-arguments Require named arguments to fix pylint too-many-positional-arguments Oct 4, 2024
@GraemeWatt
Copy link
Member

@clelange : thanks for addressing #272. I noticed the problem yesterday, but I didn't get around to opening a PR. Although it's better programming practice, it's a breaking change to reassign some arguments from positional to keyword-only. Do you anticipate this will break user code? It might be better in the long run to reassign some arguments and I leave the decision to you, but a simple fix would be to add # pylint: disable=too-many-positional-arguments to the offending functions.

@clelange
Copy link
Collaborator Author

clelange commented Oct 7, 2024

Hi @GraemeWatt -- while making some of the changes, I was also thinking about whether this breaks user code or not. So far, it looks like it's only smaller things that would be easy to fix. Forcing more named function parameters would actually make the code better readable and usable, so I'm somewhat in favour of implementing the fixes rather than silencing the linter.

@clelange
Copy link
Collaborator Author

clelange commented Oct 7, 2024

It really looks like the changes are minor and good to have even though it might break for some edge cases where people don't like named arguments.

@clelange clelange marked this pull request as ready for review October 7, 2024 14:23
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.42%. Comparing base (79091d0) to head (ab46b9d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #273   +/-   ##
=======================================
  Coverage   90.42%   90.42%           
=======================================
  Files           5        5           
  Lines        1117     1117           
  Branches      254      254           
=======================================
  Hits         1010     1010           
  Misses         78       78           
  Partials       29       29           
Flag Coverage Δ
unittests-3.10 90.42% <100.00%> (ø)
unittests-3.11 90.42% <100.00%> (ø)
unittests-3.6 90.14% <100.00%> (ø)
unittests-3.7 90.14% <100.00%> (ø)
unittests-3.8 90.24% <100.00%> (ø)
unittests-3.9 90.24% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clelange clelange requested a review from GraemeWatt October 8, 2024 19:02
Copy link
Member

@GraemeWatt GraemeWatt left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I agree that it is better practice not to allow keyword arguments to be given as positional arguments.

@clelange clelange merged commit bd71273 into main Oct 9, 2024
26 checks passed
@clelange clelange deleted the pylint_too_many_positional_arguments branch October 9, 2024 11:51
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.

Tests fail with pylint >= 3.3.0
2 participants