-
Notifications
You must be signed in to change notification settings - Fork 241
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
Update to pyparsing>=3.0.0 #480
Conversation
noxfile.py
Outdated
# Install current package and dependencies | ||
session.install(".") |
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.
Why is this necessary?
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.
As a general note: Please include a body to your commit messages, describing why a change is being made.
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.
@pradyunsg To enforce using the correct dependency versions. Installing nox
-> pytest
-> packaging
-> pyparsing<3
.
https://github.com/moyiz/packaging/runs/4054317198?check_suite_focus=true
Same goes for build
package.
Edit: Updated commit messages accordingly. Thanks.
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.
But why does it matter? We're not running packaging during the lint step and even if we did nox would've installed it in a venv separate from nox itself.
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.
@layday nox
indeed creates a virtualenv, but pyparsing
version seems to be overridden in python -m pip install build twine
since packaging
is a dependency of build
and the latest release forces pyparsing<3
. This session.install(".")
is required to enforce the current requirements of packaging
, rather than latest release.
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.
That doesn't answer the why :)
I think I understand what is going on here. Because this project doesn't use a src
-based tree, the copy of packaging
from the working directory takes precedence over the one from the nox venv. So with packaging
now only supporting pyparsing >= 3
, build
crashes.
Instead of pre- or re-installing packaging
I would suggest using the build
console script, which would prevent the cwd
from being added to the Python path:
diff --git a/noxfile.py b/noxfile.py
index 956201a..1056410 100644
--- a/noxfile.py
+++ b/noxfile.py
@@ -61,7 +61,7 @@ def lint(session):
# Check the distribution
session.install("build", "twine")
- session.run("python", "-m", "build")
+ session.run("pyproject-build")
session.run("twine", "check", *glob.glob("dist/*"))
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.
@layday Awesome catch, thanks!
Removed the re-installations in favor of the above console script.
Add support to `pyparsing>=3.0.0`. Changes required: - `originalTextFor` -> `original_text_for` - `stringEnd` -> `string_end` - `stringStart` -> `string_start` Update UT: `test_parseexception_error_msg` to check for `string_end` instead of `stringEnd` in exception message.
Install current `packaging` and its dependencies during lint. `nox` depends on `packaging`, by installing `nox` during lint phase, `packaging` dependencies defined in working branch are overridden by latest `packaging` release dependencies. In this case, since the PR changes the constraints on `pyparsing`, without a `session.install(".")` the incorrect `pyparsing` version will be installed.
This ensures that we install the dependencies of current working branch of `packaging`. Similar to previous commit. `build` package depends on `packaging` and installing it overrides the dependencies of current working branch by the dependencies of latest `packaging` release.
Can we continue to support 2 & 3 for a while? This is such a widely used package (like for nox) so I'd like it not to be restrictive. If someone does set <3, which is a bad idea, now we'll conflict the other way. We would be the better ones, but why not keep both for a while until more packages are up to 3+ only too? |
stringEnd, | ||
stringStart, |
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.
Could check the version then import stringStart as string_start.
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.
@henryiii Thanks for the suggestion.
It raises a general question regarding major upgrades of dependencies - how to determine when the adapter snippet is no longer needed? Or should the requirement be updated to >=3.0.0
in the future, after a "considerable" time of it being released?
Regarding the check itself, I can check pyparsing.__version__
prefix.
There is also the UT, I could add a selector there as well for the correct expected message according to installed version.
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.
I'd be tempted to do:
try:
from pyparsing import string_start
except ModuleNotFoundError:
from pyparsing import stringEnd as string_start
Unlike Python version, you can't statically check pyparsing.__version__
anyway. The nice thing about checking version is it provides documentation about why the compatibility bit is there.
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.
@henryiii pyparsing
is indeed shipped with __version__
, so for readability this works:
if pyparsing.__version__.startswith("2."):
from pyparsing import (
originalTextFor as original_text_for,
stringEnd as string_end,
stringStart as string_start,
)
else:
from pyparsing import (
original_text_for,
string_end,
string_start,
)
There are other options such as using pkg_resources.get_distribution
flake8
does not like this notation: N813 camelcase 'stringEnd' imported as lowercase 'string_end'
so I would have to add N813 to the ignore 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.
Why do you need a conditional import? originalTextFor
and friends exist under pyparsing 3. Do they emit a warning or something?
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.
Backward-compatibility synonyms for all names and arguments have been included, to allow parsers written using the old names to run without change. The synonyms will be removed in a future release. New parser code should be written using the new PEP-8 snake case names.
I am not sure whether "future release" will fall under 3.X.X or not.
Support `pyparsing` 2 and 3. Related changes in `pyparsing==3.0.0`: - `originalTextFor` -> `original_text_for` - `stringEnd` -> `string_end` - `stringStart` -> `string_start` Implemented according to this suggestion: pypa#480 (comment) Determine which attributres to import according to `pyparsing.__version__`, and import pyparsing 2 attributes as 3. Update UT: `test_parseexception_error_msg` to check for either `string_end` or `stringEnd` in exception message.
Support `pyparsing` 2 and 3. Related changes in `pyparsing==3.0.0`: - `originalTextFor` -> `original_text_for` - `stringEnd` -> `string_end` - `stringStart` -> `string_start` Implemented according to this suggestion: pypa#480 (comment) Determine which attributres to import according to `pyparsing.__version__`, and import pyparsing 2 attributes as 3. Update UT: `test_parseexception_error_msg` to check for either `string_end` or `stringEnd` in exception message.
Support `pyparsing` 2 and 3. Related changes in `pyparsing==3.0.0`: - `originalTextFor` -> `original_text_for` - `stringEnd` -> `string_end` - `stringStart` -> `string_start` Implemented according to this suggestion: pypa#480 (comment) Determine which attributres to import according to `pyparsing.__version__`, and import pyparsing 2 attributes as 3. Update UT: `test_parseexception_error_msg` to check for either `string_end` or `stringEnd` in exception message.
Why does the fact that there’s a constraint matter? There’s a compatible version available either way, and going forward, users would need to install the newer version. |
This is a very basic package, used in many applications. It's really nice to have a bit of overlap for a while; forcing users to update to pyparsing 3 just to get an important fix for packaging in isn't ideal. Giving a bit of time supporting both versions (especially since it's really easy to do, only a small rename with an explicit backport that does not produce a deprecation warning). Also this gets vendored into a lot of packages (pip, poetry, etc), and those vendors probably do not respect the requirements listed. Having a bit of overlap makes updating there easier. Having a high lower bound is still dramatically better than having an upper bound, but ideally, a downstream user shouldn't have to know about or care that there's a dependency on pyparsing. Keeping this supporting both versions for a little while (say till packaging 22?) would reduce then chance of problems downstream. That's my thought process, anyway. |
Support `pyparsing` 2 and 3. Related changes in `pyparsing==3.0.0`: - `originalTextFor` -> `original_text_for` - `stringEnd` -> `string_end` - `stringStart` -> `string_start` Implemented according to this suggestion: pypa#480 (comment) Determine which attributres to import according to `pyparsing.__version__`, and import pyparsing 2 attributes as 3. Update UT: `test_parseexception_error_msg` to check for either `string_end` or `stringEnd` in exception message.
See: https://github.com/pypa/packaging/pull/480/files#r740158146 As suggested by @layday, local `packaging` package directory takes precedence over the installed `packaging`. By building with `pyproject-build`, CWD will not be added to python path.
How would you prefer to proceed? The main options seem to be:
|
For option 1, what's "UT"? As for breaking when names change, that's simply part of having a dependency and we will require an update when that day comes. For option 2, that's definitely a possibility. We can't necessarily expect to always be able to have backwards-compatibility with all of our dependencies. For option 3, you can set to ignore coverage for the conditional imports to keep 100% coverage (that's what we did when we had Python 2 support). |
UT is unit test I suppose. |
Yes. |
It is very harsh to not offer a little overlap in dependency versions; someone using packaging shouldn't be forced to update pyparsing to get the latest packaging fixes; in fact, ideally, they shouldn't have to know about packaging's dependency on pyparsing at all. Being extremely strict with dependencies causes them to be a leaky abstraction; users can't freely select versions without discovering the dependency limitations. Given that pyparsing has done quite a bit to keep the transition simple, I think it would be nice to support both 2 and 3 for a little bit. Going from only supporting one major version to only supporting another major version is harsh, and requires users to sync their pyparsing upgrade with us. We technically already support both 2 and 3, but packaging 21.2 was released with no other changes except limiting pyparsing <3 just because we have one unit test that checks the text of a printed exception, which has changed in 3. If you really want to bump the minimum version to 3, that old version should be yanked. This is a temporary workaround to ease downstream updates, and we could drop support for pyparsing 2 with packaging 22.0. PS: when pip encounters an upper limit, does it look back through history to find one without that upper limit to complete the solve? If that's the case, adding an upper limit is useless after a version has been released without that limit; everyone with pyparsing 3 will get 21.1 - older versions are even less likely to be compatible, generally. If it's not the case, then it will break users who set pyparsing >=3, which is exactly what we are proposing to do here. |
See discussion in : pypa#480 `pyparsing` is backward compatible for now (synonyms for camel cased attributes). Fix unit test to accecpt either pyparsing 2 or 3 exception message.
Thanks @henryiii. Going with option 1. |
As pointed out by @henryiii in the previous PR (#471), the package should be updated and not capped.
I removed the capping restriction, set minimum version to be
3.0.0
, updated usages and said UT.