-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improve end of url detection #23
Conversation
regex_assert.finds_url('http://example.com{blah', 'http://example.com') | ||
regex_assert.finds_url('http://example.com{blah}', 'http://example.com') | ||
regex_assert.finds_url('http://example.com/foo{', 'http://example.com/foo') | ||
|
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.
This should be a separate test and probably use @pytest.mark.parametrize
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'm making the change, thanks for letting me know about @pytest.mark.parametrize
. Now each test case shows up individually!
@@ -5,7 +5,7 @@ | |||
def main(): | |||
setup( | |||
name='yelp_uri', | |||
version='1.2.0', | |||
version='2.0.0', |
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 don't see any breaking changes here?
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.
Buck promoted tlds module to its own package in a separate pull request and those changes have already been merged. You're correct, these changes are only patches and not major/minor. I can remove this and let whoever pushes the package to handle bumping the version, is that the correct procedure?
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's what I'd do. I generally avoid bumping versions in PRs anyway and leave it up to the maintainers to change the number and release (even when I'm the maintainer). It also has the added benefit of avoiding merge conflicts with others :)
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.
This has been removed from the PR.
e882e41
to
22f88f3
Compare
This is for the detection of uri to avoid including brackets and braces. Previously,
http://example.com[
was being picked up as the whole url.