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

Update to pyparsing>=3.0.0 #480

Closed
wants to merge 5 commits into from
Closed

Update to pyparsing>=3.0.0 #480

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0039779
Bump pyparsing, update usages and update UT
moyiz Oct 30, 2021
478d8a0
session.install for lint
moyiz Oct 30, 2021
b8e9b5c
Install current packaging before build
moyiz Oct 30, 2021
c319f2a
Remove re-installation in favor of stateless build
moyiz Nov 1, 2021
45cf08f
Merge branch 'main' into pyparsing_ge_3
brettcannon Nov 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ jobs:
- name: Install dependencies
run: python -m pip install --upgrade build

- name: Install package and dependencies
run: python -m pip install -e .

- name: Build
run: python -m build

Expand Down
3 changes: 3 additions & 0 deletions noxfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ def lint(session):
session.install("pre-commit")
session.run("pre-commit", "run", "--all-files")

# Install current package and dependencies
session.install(".")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member

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.

https://chris.beams.io/posts/git-commit/#why-not-how

Copy link
Contributor Author

@moyiz moyiz Oct 30, 2021 •
edited

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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/*"))

Copy link
Contributor Author

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.


# Check the distribution
session.install("build", "twine")
session.run("python", "-m", "build")
Expand Down
6 changes: 3 additions & 3 deletions packaging/markers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
ParseResults,
QuotedString,
ZeroOrMore,
stringEnd,
stringStart,
Comment on lines -19 to -20
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

@moyiz moyiz Nov 1, 2021 •
edited

Choose a reason for hiding this comment

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

@layday From changelog:

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.

string_end,
string_start,
)

from .specifiers import InvalidSpecifier, Specifier
Expand Down Expand Up @@ -135,7 +135,7 @@ def serialize(self) -> str:
MARKER_ATOM = MARKER_ITEM | Group(LPAREN + MARKER_EXPR + RPAREN)
MARKER_EXPR << MARKER_ATOM + ZeroOrMore(BOOLOP + MARKER_EXPR)

MARKER = stringStart + MARKER_EXPR + stringEnd
MARKER = string_start + MARKER_EXPR + string_end


def _coerce_parse_result(results: Union[ParseResults, List[Any]]) -> List[Any]:
Expand Down
12 changes: 6 additions & 6 deletions packaging/requirements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
Regex,
Word,
ZeroOrMore,
originalTextFor,
stringEnd,
stringStart,
original_text_for,
string_end,
string_start,
)

from .markers import MARKER_EXPR, Marker
Expand Down Expand Up @@ -63,10 +63,10 @@ class InvalidRequirement(ValueError):
_VERSION_SPEC = Optional((LPAREN + VERSION_MANY + RPAREN) | VERSION_MANY)
_VERSION_SPEC.setParseAction(lambda s, l, t: t._raw_spec or "")

VERSION_SPEC = originalTextFor(_VERSION_SPEC)("specifier")
VERSION_SPEC = original_text_for(_VERSION_SPEC)("specifier")
VERSION_SPEC.setParseAction(lambda s, l, t: t[1])

MARKER_EXPR = originalTextFor(MARKER_EXPR())("marker")
MARKER_EXPR = original_text_for(MARKER_EXPR())("marker")
MARKER_EXPR.setParseAction(
lambda s, l, t: Marker(s[t._original_start : t._original_end])
)
Expand All @@ -78,7 +78,7 @@ class InvalidRequirement(ValueError):

NAMED_REQUIREMENT = NAME + Optional(EXTRAS) + (URL_AND_MARKER | VERSION_AND_MARKER)

REQUIREMENT = stringStart + NAMED_REQUIREMENT + stringEnd
REQUIREMENT = string_start + NAMED_REQUIREMENT + string_end
# pyparsing isn't thread safe during initialization, so we do it eagerly, see
# issue #104
REQUIREMENT.parseString("x[]")
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
author=about["__author__"],
author_email=about["__email__"],
python_requires=">=3.6",
install_requires=["pyparsing>=2.0.2,<3"], # Needed to avoid issue #91
install_requires=["pyparsing>=3.0.0"],
classifiers=[
"Development Status :: 5 - Production/Stable",
"Intended Audience :: Developers",
Expand Down
2 changes: 1 addition & 1 deletion tests/test_requirements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,4 @@ def test_sys_platform_linux_in(self):
def test_parseexception_error_msg(self):
with pytest.raises(InvalidRequirement) as e:
Requirement("toto 42")
assert "Expected stringEnd" in str(e.value)
assert "Expected string_end" in str(e.value)