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

No longer require that the number of targets match requirements #234

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

brenns10
Copy link
Contributor

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 #230.

Copy link
Owner

@netromdk netromdk left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! 👍🏻

This functionality has been talked about before, and I have hesitated because it changes the behavior of the program. But on the other hand, the current functionality seems to be confusing and hard to use. So it might be a good idea to improve it.

The important thing here is to ensure that Vermin still functions as it is supposed to with all its many use cases.

Please add unit tests of the new function, compare_requirements(), in tests/general.py for when:

  • Target major version missing from requirements
  • v2 and v3 are specified
  • Only v3 is specified
  • Only v2 is specified
  • Nothing is specified
  • Some invalid values are specified
  • And other cases, if you can think of any

vermin/arguments.py Show resolved Hide resolved
vermin/arguments.py Outdated Show resolved Hide resolved
vermin/utility.py Outdated Show resolved Hide resolved
vermin/utility.py Outdated Show resolved Hide resolved
@brenns10 brenns10 force-pushed the fix_mismatched_requirements branch 2 times, most recently from a7df19d to 7493521 Compare October 18, 2023 17:11
@brenns10
Copy link
Contributor Author

Thanks for taking a look! I added unit tests for compare_requirements() in a separate commit, which should cover all the cases you mentioned, and also covers cases regarding exact vs non-exact version matching.

I didn't update or add any tests regarding the main() function, I don't know if you'd like to see a more integration-style test there as well.

Copy link
Owner

@netromdk netromdk left a comment

Choose a reason for hiding this comment

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

This looks great. I just need to test it a little further before merging.

@netromdk
Copy link
Owner

@brenns10 would you mind rebasing on top of my master branch? I just pushed a fix for the Windows test error.

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>
The tests are divided into three categories: one for the case where the
code is Python 2 and 3 compatible, and one each for the cases where the
code is exclusively Python 2 or 3 compatible.

We cover the basic cases of exercising exact and non-exact target
versions. We also cover the issues that come about when the targets can
be missing, or incorrect.

Signed-off-by: Stephen Brennan <stephen@brennan.io>
@brenns10
Copy link
Contributor Author

Done!

@coveralls
Copy link

Coverage Status

coverage: 99.756% (+0.002%) from 99.754% when pulling 0c8574c on brenns10:fix_mismatched_requirements into a0647aa on netromdk:master.

@apizarro-paypal
Copy link

apizarro-paypal commented Nov 21, 2023

@brenns10 @netromdk Really looking forward to this change. It just bit us today after adding Vermin to our CI.

@netromdk netromdk merged commit b06c831 into netromdk:master Nov 24, 2023
22 checks passed
@netromdk
Copy link
Owner

There we go. Merged 🎉
I will prepare the release of version 1.6.0 during this weekend.

@apizarro-paypal
Copy link

@netromdk Thank you so much!

@brenns10 brenns10 deleted the fix_mismatched_requirements branch November 24, 2023 17:27
@netromdk
Copy link
Owner

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

Successfully merging this pull request may close these issues.

Vermin reports that code is incompatible but it is
4 participants