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

pre-commit: increase vermin version #5173

Merged
merged 2 commits into from May 16, 2023

Conversation

chu11
Copy link
Member

@chu11 chu11 commented May 16, 2023

In #5055 some code that was only supported in Python 3.11 did not get caught by the vermin check, which surprised me. It ended up it was caught in a newer version of vermin.

So lets increase the version and add a novermin comment for one place that needs it. Also add a comment to the place that needs the novermin override.

Problem: vermin did not detect some "obvious" version incompatability
issues.

Update the pre-commit check version of vermin from 1.4.2 to 1.5.1.
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Great, thanks! Just one nit in a comment, I don't think tomllib was added until Python 3.11...

@@ -30,8 +30,10 @@

import yaml

# tomllib added to standard library in Python 3.10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# tomllib added to standard library in Python 3.10
# tomllib added to standard library in Python 3.11

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i got conflicting information about 3.10 vs 3.11. Seems we got it wrong in this prior commit too :-( 6298a36. But googling it does seem it is 3.11.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, oops!

Problem: tomllib is included on Python 3.11 and newer versions and
is loaded when available.  This can cause a vermin check failure
because the minimum required version for flux-core is 3.6.

Add a "novermin" comment to pass the vermin check.  Add a small comment
to explain code as well.
@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Merging #5173 (723be9a) into master (5b0897b) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 723be9a differs from pull request most recent head 3b9b6e3. Consider uploading reports for the commit 3b9b6e3 to get more accurate results

@@            Coverage Diff             @@
##           master    #5173      +/-   ##
==========================================
- Coverage   83.12%   83.10%   -0.02%     
==========================================
  Files         453      453              
  Lines       77856    77856              
==========================================
- Hits        64717    64705      -12     
- Misses      13139    13151      +12     
Impacted Files Coverage Δ
src/bindings/python/flux/util.py 95.21% <100.00%> (ø)

... and 12 files with indirect coverage changes

@chu11
Copy link
Member Author

chu11 commented May 16, 2023

thanks, setting MWP.

@mergify mergify bot merged commit 9cc7782 into flux-framework:master May 16, 2023
31 of 32 checks passed
@chu11 chu11 deleted the vermin_version_update branch May 16, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants