-
Notifications
You must be signed in to change notification settings - Fork 136
Move and refactor remove_base to unresolve function in iri_resolver #216
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
base: master
Are you sure you want to change the base?
Conversation
65201a2 to
fbe2bcf
Compare
d530c86 to
f480961
Compare
davidlehn
left a comment
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.
Needs a changelog entry. Could this be a breaking change?
| assert resolve('?y','http://a/bb/ccc/./d;p?q') == 'http://a/bb/ccc/./d;p?y' | ||
|
|
||
| # ---------- Tests for unresolve() ---------- | ||
| class TestUnresolve: |
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.
Should class-based or function-based pytest testcases be chosen as uniform practice for this project?
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 personally prefer class-based, but I'm fine with both. The only reason why the specs are function-based, is because I couldn't figure out how to make it work otherwise :) What do you think?
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 must confess I never saw pytest class based tests in prod code. Everyone just writes functions after unittest ceased to be de facto standard. But that's just my anecdotal evidence.
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 have, so I guess it's personal preference. I suggest we keep it for now, being
- one
tests/test_*.pyfor everylib/*.py(with the exception oftest_manifests.pyofc) - one class per function that needs testing
... and having a bigger discussion on this some time later
66827a9 to
1c521e4
Compare
fd707e1 to
7f8ea97
Compare
1c521e4 to
6d81a1f
Compare
7f8ea97 to
db3a2d1
Compare
|
@davidlehn I've updated the changelog! I haven't seen any breaking changes, at least not what the spec tests are concerned. Other parts of the lib, not sure, don't know the lib well enough yet. The use of this function is pretty contained, so I think it's ok to see whether any regressions pop up and report them as new bug issues. |
Please review and merge #207 first!
This PR
remove_base(base, iri)toiri_resolver.pyand renames it tounresolve(iri, base)(in correspondence withresolve())urlparseandurlunparseremove_base((base, iri))unresolve(iri, base)intests/test_iri_parser.py