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

Relative path #425

Closed
wants to merge 9 commits into from
Closed

Conversation

alandefreitas
Copy link
Member

fix #407

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

return comp;
if (has_scheme())
{
comp = detail::ci_compare(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need this as a public function in grammar if it is a case-insensitive comparison

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where? What file?

@vinniefalco
Copy link
Member

quite a lot here !

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #425 (579deee) into develop (afb91bc) will decrease coverage by 0.07%.
The diff coverage is 98.75%.

❗ Current head 579deee differs from pull request most recent head f9c5121. Consider uploading reports for the commit f9c5121 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #425      +/-   ##
===========================================
- Coverage    97.92%   97.84%   -0.08%     
===========================================
  Files          132      132              
  Lines         6349     6494     +145     
===========================================
+ Hits          6217     6354     +137     
- Misses         132      140       +8     
Impacted Files Coverage Δ
include/boost/url/url.hpp 100.00% <ø> (ø)
include/boost/url/impl/url.ipp 97.04% <98.11%> (+1.80%) ⬆️
include/boost/url/authority_view.hpp 94.87% <100.00%> (+0.58%) ⬆️
include/boost/url/impl/authority_view.ipp 96.77% <100.00%> (+2.03%) ⬆️
include/boost/url/impl/url_base.ipp 98.87% <100.00%> (ø)
include/boost/url/impl/url_view_base.ipp 98.56% <100.00%> (-1.44%) ⬇️
include/boost/url/url_view_base.hpp 100.00% <100.00%> (ø)
include/boost/url/detail/impl/normalize.ipp 97.46% <0.00%> (-1.90%) ⬇️

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 afb91bc...f9c5121. Read the comment docs.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@alandefreitas
Copy link
Member Author

quite a lot here !

Yes. This function seems useful. The PR was also useful to find lots of small bugs and features missing here and there. Like some problems with the url_base::ops and set_path(""). It also gave me some ideas to improve that noexcept path comparison we have in url_view::compare.

Some other questions that came up:

  • About the order of arguments: std::filesystem/boost::filesystem functions use relative(const path&, const path& base). Also absolute, proximate, etc. It does look more natural. Shouldn't we do the same here to relative/absolute/resolve?

  • The basic exception guarantee is not satisfied in case of allocation errors after the first allocation. Is that a problem?

@alandefreitas
Copy link
Member Author

Oh... look what URI.js says about absoluteTo:

https://github.com/medialize/URI.js/blob/28976cde5bb32a433ab78c4a32ef58f39d2cabf6/test/test.js#L1479

So we only need resolve and relative.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@alandefreitas alandefreitas force-pushed the relative_path branch 2 times, most recently from 8e3f0e4 to 9ad001f Compare August 19, 2022 20:50
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@alandefreitas alandefreitas changed the title Relative path [DRAFT] Relative path Aug 19, 2022
@alandefreitas
Copy link
Member Author

I fixed the last test that was failing:

check("/../path/x/../to/y/../somewhere/else", "/a/../../path/to/a", "../a");

This function had a problem similar to op==: comparing paths without a stack. The difference is we need the longest common path instead.

As usual, I tried many solutions and realized comparing the segments in reverse is the only solution with linear cost. Because of how hard this is and so we can move on, I used the other solution which could be n^2 in a very edgy case.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

1 similar comment
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@vinniefalco
Copy link
Member

we can use temporary memory with recycled_ptr now if it will simplify and speed up the algorithm

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

2 similar comments
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html


return {};
dest.copy(base);
return dest.relative(href);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relative is the name? That is kind of obscure... doesn't really suggest what it does. Why are we adding features

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relative is the name? That is kind of obscure... doesn't really suggest what it does.

URL.js uses relative_to. I can't think of anything better than relative/relative_to

Why are we adding features

I don't know. I was assigned this issue and started writing the code at some point.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

1 similar comment
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

1 similar comment
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@alandefreitas
Copy link
Member Author

I rebased this PR and fixed everything that broke with the new containers.

@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #425 (559dc25) into develop (ecdc8f1) will increase coverage by 0.01%.
The diff coverage is 98.88%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #425      +/-   ##
===========================================
+ Coverage    96.81%   96.83%   +0.01%     
===========================================
  Files          139      139              
  Lines         6691     6847     +156     
===========================================
+ Hits          6478     6630     +152     
- Misses         213      217       +4     
Impacted Files Coverage Δ
include/boost/url/url.hpp 100.00% <ø> (ø)
include/boost/url/impl/url_base.ipp 98.95% <98.19%> (-0.08%) ⬇️
include/boost/url/authority_view.hpp 94.59% <100.00%> (+0.65%) ⬆️
include/boost/url/impl/authority_view.ipp 84.17% <100.00%> (+5.31%) ⬆️
include/boost/url/impl/url_view_base.ipp 98.60% <100.00%> (+<0.01%) ⬆️
include/boost/url/url_base.hpp 100.00% <100.00%> (ø)
include/boost/url/detail/impl/normalize.ipp 97.46% <0.00%> (-1.90%) ⬇️

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 ecdc8f1...559dc25. Read the comment docs.

@cppalliance-bot
Copy link

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@cppalliance-bot
Copy link

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://425.url.prtest.cppalliance.org/libs/url/doc/html/index.html

@cppalliance-bot
Copy link

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

Successfully merging this pull request may close these issues.

relative_to and absolute_to
3 participants