-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
More consistency checks for dependencies #4990
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
Conversation
def _strip_dep_version(dependency): | ||
dep_version_pos = len(dependency) | ||
for pos, c in enumerate(dependency): | ||
if c in "=<>": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is a semicolon? Example from mypy test-requirements.txt
:
flake8-bugbear; python_version >= '3.5'
We check for spaces, but somebody might write something like this:
types-foo;python_version>='3.5'
Maybe use a regexp to validate the version string so that we don't check for bad things but ensure that only expected things exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, there should not be semicolons, requires
is already a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't notice what is in the second part, I think we may not support this initially.
return stripped, "", "" | ||
number_pos = 0 | ||
for pos, c in enumerate(rest): | ||
if c not in "=<>": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one wording suggestion below.
tests/check_consistent.py
Outdated
for space in " \t\n": | ||
assert space not in dep, f"For consistency dependency should not have whitespace: {dep}" | ||
stripped, relation, dep_version = _strip_dep_version(dep) | ||
assert stripped in known_distributions, f"Only known dependencies are supported, got {stripped}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe "Only dependencies from typeshed are supported, got {stripped}"
Opened #4995 as a follow-up from offline discussions. |
Closes #4988