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

Fix erroneous non-ASCII in local version #470

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

zuo
Copy link

@zuo zuo commented Oct 23, 2021

Closes: #469.

  • Fix validation of the given version in the packaging.version.Version's constructor.
  • Fix validation of the given spec in the packaging.specifiers.Specifier's constructor.
  • Complement relevant tests, including adding cases related to presence of non-ASCII whitespace characters in input strings.
  • Fix the docs of packaging.version.VERSION_PATTERN by mentioning necessity
    of the re.ASCII flag.

@zuo zuo changed the title Fix erroneous non-ASCII in local version (#469) Fix erroneous non-ASCII in local version Oct 23, 2021
packaging/version.py Outdated Show resolved Hide resolved
packaging/version.py Outdated Show resolved Hide resolved
packaging/specifiers.py Outdated Show resolved Hide resolved
@zuo
Copy link
Author

zuo commented Oct 26, 2021

@pradyunsg @brettcannon

Hmm, the error (some stringEnd vs. string_end discrepancy in an InvalidRequirement's exception message) that is reported by automatic checks seems unrelated to this PR.

=================================== FAILURES ===================================
________________ TestRequirements.test_parseexception_error_msg ________________

self = <tests.test_requirements.TestRequirements object at 0x00007f89be140fe0>

    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'

@uranusjr
Copy link
Member

uranusjr commented Oct 26, 2021

I think we also need tests to validate non-ASCII whitespaces are removed correctly.

* Fix validation in the packaging.version.Version's constructor

* Fix validation in the packaging.specifiers.Specifier's constructor

* Complement relevant tests, including adding cases related to presence
  of non-ASCII whitespace characters in input strings

* Fix docs of packaging.version.VERSION_PATTERN by mentioning necessity
  of the re.ASCII flag
@zuo zuo force-pushed the fix-ascii-local-version branch from a5bfabf to 89ee170 Compare October 26, 2021 22:07
@zuo
Copy link
Author

zuo commented Oct 26, 2021

@uranusjr

I think we also need tests to validate non-ASCII whitespaces are removed correctly.

OK, added.

CHANGELOG.rst Outdated Show resolved Hide resolved
@zuo zuo requested a review from pradyunsg October 28, 2021 19:02
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
@zuo zuo force-pushed the fix-ascii-local-version branch from 4e76ac3 to 83b8ac6 Compare October 28, 2021 21:34
@zuo
Copy link
Author

zuo commented Nov 13, 2021

I am sorry, I miss-clicked so that these 3 workflows become required again. :-(

@pradyunsg
Copy link
Member

You didn't misclick -- you did all the right things. :)

It's just that GitHub requires me to click a button every time you modify a PR (It's their way to avoiding cryptomining endevours) to run our checks and I've done that now!

@zuo
Copy link
Author

zuo commented Nov 15, 2021

Ah, ok. :-)

@zuo
Copy link
Author

zuo commented Nov 23, 2021

What else do I need to do?

@brettcannon
Copy link
Member

What else do I need to do?

At this point probably just be patient. We are all busy and so it might be a while until someone has time to review this PR.

@brettcannon
Copy link
Member

@zuo I've got time to review this now (sorry about the delay 😅), but there are now merge conflicts. Did you want to update the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version and Specifier accept (erroneously) some non-ASCII letters in the *local version* segment
4 participants