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

attempt at a fix for the rooted flag #91

Merged
merged 3 commits into from
Nov 21, 2019
Merged

attempt at a fix for the rooted flag #91

merged 3 commits into from
Nov 21, 2019

Conversation

glyph
Copy link
Collaborator

@glyph glyph commented Nov 10, 2019

Fixes #90

@codecov-io
Copy link

codecov-io commented Nov 10, 2019

Codecov Report

Merging #91 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   98.51%   98.53%   +0.02%     
==========================================
  Files          10       10              
  Lines        1546     1568      +22     
  Branches      177      179       +2     
==========================================
+ Hits         1523     1545      +22     
  Misses         12       12              
  Partials       11       11
Impacted Files Coverage Δ
src/hyperlink/_url.py 96.93% <100%> (+0.01%) ⬆️
src/hyperlink/test/test_url.py 99.81% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d33145...56b1dd3. Read the comment docs.

@glyph
Copy link
Collaborator Author

glyph commented Nov 19, 2019

@wsanchez @mahmoud anybody up for a review

@wsanchez
Copy link
Contributor

This looks fine to me, given the way the code works now, but I guess I don't understand why rooted is an argument to URL in the first place.

Shouldn't it just be a computed (perhaps cached) property instead of a user-defined attribute?

@wsanchez
Copy link
Contributor

I guess my question is really just aspirational; the API is what it is.

@glyph
Copy link
Collaborator Author

glyph commented Nov 21, 2019

I guess my question is really just aspirational; the API is what it is.

I think the question still deserves answering, since this API is super subtle. The main thing I'd say about this is that it does make a lot more sense than attempting to deal with nonsensical values for path, but still always get the right number of slashes, and not use different data structures for relative and absolute URLs.

@glyph glyph merged commit c0dd684 into master Nov 21, 2019
@glyph glyph deleted the rooted-fix branch November 21, 2019 04:22
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.

rooted flag exposes serialization ambiguity
3 participants