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

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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:
run: python -m pip install --upgrade build

- name: Build
run: python -m build
run: pyproject-build

- name: Archive files
uses: actions/upload-artifact@v1
Expand Down
2 changes: 1 addition & 1 deletion noxfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/*"))


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
Loading

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)