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

Check for number of detected minimum versions seems not useful #79

Closed
bbannier opened this issue Oct 22, 2021 · 4 comments
Closed

Check for number of detected minimum versions seems not useful #79

bbannier opened this issue Oct 22, 2021 · 4 comments

Comments

@bbannier
Copy link

bbannier commented Oct 22, 2021

Describe the bug

If I run vermin with flags requiring a single minimum version on a file that supports multiple versions a warning is triggered. This warning doesn't seem useful.

# foo.py
# empty
$ vermin --target='3.10-' -vv --violations foo.py
Detecting python files..
Analyzing using 16 processes..
Tip: Since '# novm' or '# novermin' weren't used, a speedup can be achieved using: --no-parse-comments
Minimum required versions: ~2, ~3
Target versions not met:   3.10-
Note: Number of specified targets (1) doesn't match number of detected minimum versions (2).

The issue goes away if I actually add a file removing 2 from the minimum required set,

# bar.py
X = 'foo'
f"{X}"
vermin --target='3.10-' -vv --violations foo.py bar.py
Detecting python files..
Analyzing 2 files using 16 processes..
Tip: Since '# novm' or '# novermin' weren't used, a speedup can be achieved using: --no-parse-comments
Minimum required versions: 3.6
Incompatible versions:     2

The warning when running over a single file does not seem useful and even potentially harmful.

Expected behavior

I would like to configure vermin for pre-commit. For each Git commit pre-commit runs on the set of changed files. I had expected that I can configure vermin in a way so that violations of a single file are a subset of the violations of all files. The current behavior seems to be the opposite.

Environment (please complete the following information):

  • vermin-1.3.0
@netromdk
Copy link
Owner

netromdk commented Oct 23, 2021

Thanks for reaching out.

For context, via --help:

Results interpretation:
  ~2       No known reason it won't work with py2.
  !2       It is known that it won't work with py2.
  2.5, !3  Works with 2.5+ but it is known it won't work with py3.
  ~2, 3.4  No known reason it won't work with py2, works with 3.4+

The result of

Minimum required versions: ~2, ~3

means that it doesn't have any evidence that it won't work with v2.x and v3.x - the file is empty. It doesn't mean that it infers it can run on v2.x and v3.x. Btw it also exits with code 1 in this case since Target versions not met: 3.10-.

But as soon as any data is found it can then conclude that it's not compatible with v2.x and requires v3.6+ via "bar.py".

Would it be more clear if it showed an extra note at the end when having either ~2 and/or ~3, like the following?

Note: Not enough evidence to conclude it won't work with py2.             (for ~2)
Note: Not enough evidence to conclude it won't work with py3.             (for ~3)
Note: Not enough evidence to conclude it won't work with py2 and py3.     (for ~2 and ~3)

And then maybe not showing

Note: Number of specified targets (1) doesn't match number of detected minimum versions (2).

when having ~2 and/or ~3 - that doesn't make much sense since those are things it cannot conclude on concretely.

So wrapping up, your first run on "foo.py" could then yield the following to be more clear:

$ vermin --target='3.10-' -vv --violations foo.py
Detecting python files..
Analyzing using 16 processes..
Tip: Since '# novm' or '# novermin' weren't used, a speedup can be achieved using: --no-parse-comments
Minimum required versions: ~2, ~3
Note: Not enough evidence to conclude it won't work with py2 and py3.
Target versions not met:   3.10-

Interesting idea to use on Git pre-commit. Should work fine. If your target version is violated, it'll exit with 1. I do see the point: if there isn't enough data to conclude a minimum version then the target version will fail, but you'd prefer that it didn't fail in that particular scenario since both are ~2 and ~3?

@bbannier
Copy link
Author

We document a minimum required Python version and I would like to check that no newer language features are used. I understand the diagnostic messages, but the way return codes are computed seems to break that workflow.

To recap, what I would like to do:

  • configure a minimum required version X.Y-
  • run vermin on a single, multiple files, or all files of the project
  • if use of features from X.Y+ is detected produce an error

The way I understood the flags, I think this would work if vermin returned 0 if it couldn't conclude that there's a violation (e.g., for an empty file, or if no features added in python-X.Y are used). If this cannot be the default behavior of --target for backwards compatibility reasons it could maybe be behavior produced when --violations is passed?

Or is there some other way to do what I am after?

@netromdk
Copy link
Owner

Indeed for backwards compatibility, I think it makes most sense to make this change for when using --target and --violations in conjunction. I've made a fix that I intend to release with Vermin v1.3.1 - I think this weekend or the next.

For your two scenarios, it now yields:

% ./vermin.py --target='3.10-' -vv --violations foo.py ; echo "exit code=$?"
Detecting python files..
Analyzing using 8 processes..
Tip: Since '# novm' or '# novermin' weren't used, a speedup can be achieved using: --no-parse-comments
Minimum required versions: ~2, ~3
exit code=0

% ./vermin.py --target='3.10-' -vv --violations bar.py ; echo "exit code=$?"
Detecting python files..
Analyzing using 8 processes..
Tip: Since '# novm' or '# novermin' weren't used, a speedup can be achieved using: --no-parse-comments
Minimum required versions: 3.6
Incompatible versions:     2
exit code=0

netromdk added a commit that referenced this issue Oct 24, 2021
… 0 (#79)

If no rules are triggered the minimum versions detected will be ~2 and ~3, which means nothing
conclusive can be determined and thus the target violation shouldn't mean exit code 1.
@netromdk
Copy link
Owner

@bbannier v1.3.1 has been released.

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

No branches or pull requests

2 participants