HEPData / hepdata_lib

Library for getting your data into HEPData
https://hepdata-lib.readthedocs.io
MIT License
16 stars 39 forks source link

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

Closed clelange closed 1 month ago

clelange commented 1 month ago

Fixes #272


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

clelange commented 1 month ago

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

GraemeWatt commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | Coverage Δ | | |---|---|---| | [unittests-3.10](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.42% <100.00%> (ø)` | | | [unittests-3.11](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.42% <100.00%> (ø)` | | | [unittests-3.6](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.14% <100.00%> (ø)` | | | [unittests-3.7](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.14% <100.00%> (ø)` | | | [unittests-3.8](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.24% <100.00%> (ø)` | | | [unittests-3.9](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.24% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.