Use `strict=True` with `zip` to ensure length equality#4011

Merged

janosh merged 7 commits intomaterialsproject:masterfrom

DanielYang59:zip-strict-true

Aug 22, 2024

Contributor

DanielYang59 commented Aug 22, 2024 •

edited

Loading

Summary

Use strict=True when zip to ensure length equality @janosh, to finish Drop Python 3.9 support #4009 (comment)

TODOs

Let's see how many tests are failing
Make sure strict keyword is for zip not other function


          global replacement of strict=True, very prone to error and need caref…

44841d6

…ul check

DanielYang59 commented

View reviewed changes

src/pymatgen/io/abinit/inputs.py Outdated

DanielYang59 commented

View reviewed changes

src/pymatgen/io/abinit/inputs.py Outdated

DanielYang59 commented

View reviewed changes

src/pymatgen/io/abinit/inputs.py Outdated

DanielYang59 commented

View reviewed changes

src/pymatgen/io/abinit/inputs.py Outdated

DanielYang59 commented

View reviewed changes

src/pymatgen/io/cp2k/inputs.py Outdated

DanielYang59 commented

View reviewed changes

src/pymatgen/io/cp2k/inputs.py Outdated

DanielYang59 commented

View reviewed changes

src/pymatgen/io/cp2k/inputs.py Outdated

DanielYang59 commented

View reviewed changes

tests/core/test_composition.py Outdated

DanielYang59 commented

View reviewed changes

tests/core/test_composition.py Outdated

DanielYang59 commented

View reviewed changes

tests/io/abinit/test_inputs.py Outdated

DanielYang59 commented

View reviewed changes

tests/io/aims/test_sets/test_input_set.py Outdated

DanielYang59 added 5 commits

August 22, 2024 15:08


          revert changes on non-zip functions

4ab1d34


          revert to strict=F for failing tests

89dc247


          revert a missing one

b149ebb


          remove a call-overload ignore tag

316ab1a


          fix another unit test

f4b72cd

DanielYang59 marked this pull request as ready for review

August 22, 2024 08:09

DanielYang59 requested review from janosh, mkhorton and shyuep as code owners

August 22, 2024 08:09


          fix typos

01ea40b

janosh reviewed

View reviewed changes

Member

janosh left a comment

thanks @DanielYang59! that was way faster than i expected. 👍

did you notice any cases where strict=True stood out as potentially problematic? knowing pymatgen's test coverage, i assume there are cases where strict=True wouldn't raise a CI warning despite strict=False being required for backwards compatibility

Contributor Author

DanielYang59 commented Aug 22, 2024

did you notice any cases where strict=True stood out as potentially problematic?

Sorry I didn't really go through each of these replacements (just a batch replace and fixed those CI failures in 89dc247 and b149ebb). Perhaps I should mark it as draft now and fully go through all changes later.

i assume there are cases where strict=True wouldn't raise a CI warning

With strict=True a ValueError would be raised directly instead of warning. So it would be a good thing (reveal untested code) and a bad thing (might lead to many breakages from untested code).

DanielYang59 marked this pull request as draft

August 22, 2024 09:06

Member

janosh commented Aug 22, 2024

With strict=True a ValueError would be raised directly instead of warning

sorry, i meant error, not warning

So it would be a good thing (reveal untested code) and a bad thing (might lead to many breakages from untested code).

agreed

Perhaps I should mark it as draft now and fully go through all changes later.

not strictly necessary imo. these issues are very hard to spot by just looking at the code. overall less time spent reporting and fixing errors as they occur

janosh added linting housekeeping labels

Contributor Author

DanielYang59 commented Aug 22, 2024

not strictly necessary imo. these issues are very hard to spot by just looking at the code. overall less time spent reporting and fixing errors as they occur

Yes that's what I thought... Though sounds a bit irresponsible, but it's quite hard just by eyeballing

DanielYang59 marked this pull request as ready for review

August 22, 2024 09:12

DanielYang59 changed the title ~~Use strict=True when zip to ensure length equality~~ Use strict=True with zip to ensure length equality

janosh approved these changes

View reviewed changes

Member

janosh left a comment

thanks as always @DanielYang59!

janosh merged commit 6ca78b3 into materialsproject:master

DanielYang59 deleted the zip-strict-true branch

August 22, 2024 09:19

Contributor Author

DanielYang59 commented Aug 22, 2024

No problem. Thanks for reviewing. Fingers crossed that this PR wouldn't be pinged a hundred times :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

shyuep Awaiting requested review from shyuep shyuep is a code owner

mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

1 more reviewer

janosh janosh approved these changes

Labels

housekeeping linting