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.

Sign up for GitHub

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

Jump to bottom

Vermin reports that code is incompatible but it is #230

Closed
WhyNotHugo opened this issue Sep 26, 2023 · 3 comments · Fixed by #234
Closed

Vermin reports that code is incompatible but it is #230

WhyNotHugo opened this issue Sep 26, 2023 · 3 comments · Fixed by #234

Comments

@WhyNotHugo
Copy link

Describe the bug

Vermin reports that my codebase is not compatible with 3.8, but it is. Oddly, it points out:

Minimum required versions: 2.3, 3.0

Which sounds like it should be fine to me.

To Reproduce

if sys.version_info < (3, 8):  # noqa: UP036 # novermin
    errstr = "khal only supports python version 3.8+. Please Upgrade.\n"
    sys.stderr.write("#" * len(errstr) + '\n')
    sys.stderr.write(errstr)
    sys.stderr.write("#" * len(errstr) + '\n')
    sys.exit(1)

Vermin fails, indicating that this code doesn't run for the specified version:

> vermin -t=3.8  --violations setup.py
Detecting python files..
Analyzing using 24 processes..
Minimum required versions: 2.3, 3.0
Target versions not met:   3.8
Note: Number of specified targets (1) doesn't match number of detected minimum versions (2).

Expected behavior

This should work just fine.

Environment (please complete the following information):

  • Via pre-commit, v1.5.2.
  • From Alpine packages, v1.5.2

Additional context

Full file is here: https://github.com/pimutils/khal/blob/f5ca8883f2e3d8a403d7a6d16fa4f552b6b69283/setup.py
brenns10 added a commit to brenns10/vermin that referenced this issue Oct 10, 2023
@brenns10
No longer require that the number of targets match requirements
a782d8b 
In --violations mode, the CLI requires that the number of "--target"
specified is the same as the number of requirements. Many users no
longer care about Python 2, but may write a simple file which is
compatible with Python 2 by chance. In that case, Vermin will return a
Python 2.x compatibility entry, and --violations will fail unless a
target is specified for Python 2. However, there's no way to specify a
target of "I don't care about this version", so these users would be
forced to run with something like "-t=2.7- -t=3.6-". Except this would
fail on their normal code, which is incompatible with Python 2.x.

To get around this, start comparing by the major version of Python. Only
compare the versions which are in common between the requirements and
target. If there are no versions in common, fail. If a target major
version is missing from the requirements, fail.

This allows users who only specify a "3.x-" target to know that Vermin
will succeed on any code, even simple code which is 2.x compatible too.

This fixes netromdk#230.

Signed-off-by: Stephen Brennan <stephen@brennan.io>
@brenns10 brenns10 mentioned this issue Oct 10, 2023
Merged
@brenns10
Copy link
Contributor

Yeah, I encountered the same issue - simple code which happens to be Python 2.x compatible will get flagged because I've only provided Python 3.x compatibility targets.

The issue is that there's basically a list of requirements, and a list of targets. The requirements includes an entry for Python 2.x and for 3.x. But you've only specified targets for 3.x. So when the code goes to check the requirements, it sees that the lengths of these two lists don't match, and fails. My pull request (#234) adjusts the logic to align the targets and requirements on their major version, so that the lists don't need to match in length.

brenns10 added a commit to brenns10/vermin that referenced this issue Oct 18, 2023
@brenns10
No longer require that the number of targets match requirements
555e0e0 
In --violations mode, the CLI requires that the number of "--target"
specified is the same as the number of requirements. Many users no
longer care about Python 2, but may write a simple file which is
compatible with Python 2 by chance. In that case, Vermin will return a
Python 2.x compatibility entry, and --violations will fail unless a
target is specified for Python 2. However, there's no way to specify a
target of "I don't care about this version", so these users would be
forced to run with something like "-t=2.7- -t=3.6-". Except this would
fail on their normal code, which is incompatible with Python 2.x.

To get around this, start comparing by the major version of Python. Only
compare the versions which are in common between the requirements and
target. If there are no versions in common, fail. If a target major
version is missing from the requirements, fail.

This allows users who only specify a "3.x-" target to know that Vermin
will succeed on any code, even simple code which is 2.x compatible too.

This fixes netromdk#230.

Signed-off-by: Stephen Brennan <stephen@brennan.io>
brenns10 added a commit to brenns10/vermin that referenced this issue Oct 19, 2023
@brenns10
No longer require that the number of targets match requirements
56489ef 
In --violations mode, the CLI requires that the number of "--target"
specified is the same as the number of requirements. Many users no
longer care about Python 2, but may write a simple file which is
compatible with Python 2 by chance. In that case, Vermin will return a
Python 2.x compatibility entry, and --violations will fail unless a
target is specified for Python 2. However, there's no way to specify a
target of "I don't care about this version", so these users would be
forced to run with something like "-t=2.7- -t=3.6-". Except this would
fail on their normal code, which is incompatible with Python 2.x.

To get around this, start comparing by the major version of Python. Only
compare the versions which are in common between the requirements and
target. If there are no versions in common, fail. If a target major
version is missing from the requirements, fail.

This allows users who only specify a "3.x-" target to know that Vermin
will succeed on any code, even simple code which is 2.x compatible too.

This fixes netromdk#230.

Signed-off-by: Stephen Brennan <stephen@brennan.io>
@netromdk
Copy link
Owner

@WhyNotHugo finally merged it :) thanks again, @brenns10! Version 1.6 will be released tomorrow.

@netromdk
Copy link
Owner

netromdk commented Nov 25, 2023 •
edited
Loading

Boom! https://github.com/netromdk/vermin/releases/tag/v1.6.0

brenns10 added a commit to brenns10/drgn that referenced this issue Nov 27, 2023
@brenns10
pre-commit: Update all hook versions and apply fixes
e83ab80 
Vermin 1.6.0 was recently released, which contains a fix for
netromdk/vermin#230. This was a rather odd corner case, in which Vermin
would fail when code was compatible with 2.x and 3.x, yet the command
line only required compatibility with 3.x. Since pre-commit runs against
only files that changed, this was possible, for example with small test
files. I'm not sure if I encountered it in drgn (I know I did with
drgn-tools), but it's good to have the fix.

I used pre-commit autoupdate, so we have the latest version of each hook
now. In particular, this commit surpasses mypy 0.971, which is the last
version to support Python 3.6 at runtime. Mypy still supports targeting
Python 3.6[1].

[1]: https://mypy-lang.blogspot.com/2022/07/mypy-0971-released.html

Two new errors were surfaced with the new hooks:

1. Mypy complained about the pattern "os.exit(exception)". I've replaced
   these so they explicitly use str: "os.exit(str(exception))".

drgn/cli.py:283: error: Argument 1 to "exit" has incompatible type "OSError"; expected "str | int | None"  [arg-type]

2. Vermin complained that "int.is_integer()" was introduced 3.12. I
   surmised that it was unable to infer that its usage was guaranteed to
   be against a float, and the reason was due to the division operation
   against an integer. I changed the integer literal to a float literal,
   which resolved the issue.

!2, 3.12     drgn/helpers/common/format.py
  'int.is_integer' member requires !2, 3.12

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
osandov pushed a commit to osandov/drgn that referenced this issue Nov 29, 2023
@brenns10 @osandov
pre-commit: Update all hook versions and apply fixes
c3449b8 
Vermin 1.6.0 was recently released, which contains a fix for
netromdk/vermin#230. This was a rather odd corner case, in which Vermin
would fail when code was compatible with 2.x and 3.x, yet the command
line only required compatibility with 3.x. Since pre-commit runs against
only files that changed, this was possible, for example with small test
files. I'm not sure if I encountered it in drgn (I know I did with
drgn-tools), but it's good to have the fix.

I used pre-commit autoupdate, so we have the latest version of each hook
now. In particular, this commit surpasses mypy 0.971, which is the last
version to support Python 3.6 at runtime. Mypy still supports targeting
Python 3.6[1].

[1]: https://mypy-lang.blogspot.com/2022/07/mypy-0971-released.html

Two new errors were surfaced with the new hooks:

1. Mypy complained about the pattern "os.exit(exception)". I've replaced
   these so they explicitly use str: "os.exit(str(exception))".

drgn/cli.py:283: error: Argument 1 to "exit" has incompatible type "OSError"; expected "str | int | None"  [arg-type]

2. Vermin complained that "int.is_integer()" was introduced 3.12. I
   surmised that it was unable to infer that its usage was guaranteed to
   be against a float, and the reason was due to the division operation
   against an integer. I changed the integer literal to a float literal,
   which resolved the issue.

!2, 3.12     drgn/helpers/common/format.py
  'int.is_integer' member requires !2, 3.12

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

No longer require that the number of targets match requirements brenns10/vermin
3 participants
@WhyNotHugo @brenns10 @netromdk