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

Limit pyparsing to major version 2 #471

Merged
merged 2 commits into from
Oct 27, 2021
Merged

Conversation

moyiz
Copy link
Contributor

@moyiz moyiz commented Oct 24, 2021

pyparsing released a new major version (3.0.0) with breaking changes.

Package: https://github.com/pyparsing/pyparsing/
Changelog: https://github.com/pyparsing/pyparsing/blob/master/docs/whats_new_in_3_0_0.rst
Notable changes relate to renaming originalTextFor, stringEnd, stringStart.

This change limits pyparsing to not installl / upgrade to 3.0.0 and resolves the failing test_parseexception_error_msg:

self = <tests.test_requirements.TestRequirements object at 0x7f418a028160>
                                                                               
    def test_parseexception_error_msg(self):                                                                                                                  
        with pytest.raises(InvalidRequirement) as e:                                                                                                          
            Requirement("toto 42")                            
>       assert "Expected stringEnd" in str(e.value)         
E       assert 'Expected stringEnd' in 'Parse error at "\'42\'": Expected string_end'                                                                         
E        +  where 'Parse error at "\'42\'": Expected string_end' = str(InvalidRequirement('Parse error at "\'42\'": Expected string_end'))
E        +    where InvalidRequirement('Parse error at "\'42\'": Expected string_end') = <ExceptionInfo InvalidRequirement('Parse error at "\'42\'": Expected string_end') tblen=2>.value
                                                                                                                                                              
tests/test_requirements.py:195: AssertionError   

@pradyunsg
Copy link
Member

(approved the CI run again)

@moyiz
Copy link
Contributor Author

moyiz commented Oct 24, 2021

@pradyunsg Thanks. The tests currently fail because the workflow installs pytest which depends on packaging which sneaks pyparsing==3.0.0 into the test phase. Should the GitHub action be updated to install packaging from "current" branch?

Edit: Meant nox. Added a session.install(".") to use current packaging dependencies. UTs pass in my fork now.

@pradyunsg pradyunsg merged commit 20cd09e into pypa:main Oct 27, 2021
@pradyunsg pradyunsg mentioned this pull request Oct 27, 2021
@moyiz moyiz deleted the pyparsing_lt_3 branch October 27, 2021 11:53
@henryiii
Copy link
Contributor

Capping this means that packages that use packaging cannot be installed in the same environment as any library that uses pyparsing>=3.0 - with the renames and fairly large changes, this will probably be quite popular (I was just considering it a day or two ago). This really needs to be fixed to support the newly renamed methods, not capped, not for a library like packaging. Adding a version cap does have consequences.

@henryiii
Copy link
Contributor

henryiii commented Oct 29, 2021

Also, isn't this really just breaking tests that are looking for exact strings in exception messages? Is it breaking any real code? And who's tests are it breaking other than ours?

@layday
Copy link
Member

layday commented Oct 29, 2021

This does only seem to break the one test which is comparing the error message. The release notes say:

Backward-compatibility synonyms for all names and arguments have been included, to allow parsers written using the old names to run without change.

@henryiii
Copy link
Contributor

Could we just correct this one test to look for both synonyms and then yank 21.2?

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.

4 participants