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

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

Merged
merged 7 commits into from
Aug 22, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Aug 22, 2024

Summary

TODOs

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

@DanielYang59 DanielYang59 marked this pull request as ready for review August 22, 2024 08:09
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

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

@DanielYang59
Copy link
Contributor Author

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 DanielYang59 marked this pull request as draft August 22, 2024 09:06
@janosh
Copy link
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 janosh added linting Linting and quality assurance housekeeping Moving around or cleaning up old code/files labels Aug 22, 2024
@DanielYang59
Copy link
Contributor Author

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 DanielYang59 marked this pull request as ready for review August 22, 2024 09:12
@DanielYang59 DanielYang59 changed the title Use strict=True when zip to ensure length equality Use strict=True with zip to ensure length equality Aug 22, 2024
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks as always @DanielYang59!

@janosh janosh merged commit 6ca78b3 into materialsproject:master Aug 22, 2024
33 checks passed
@DanielYang59 DanielYang59 deleted the zip-strict-true branch August 22, 2024 09:19
@DanielYang59
Copy link
Contributor Author

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
Labels
housekeeping Moving around or cleaning up old code/files linting Linting and quality assurance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants