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

feat: add join method to Url class #1378

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Meetesh-Saini
Copy link

  • added support for URL path joining with optional trailing slashes and multiple arguments.

Change Summary

This PR implements a feature based on pydantic/pydantic#9794 to join URL path into the base URL. It uses the join method from the url crate.

Related issue number

fix pydantic/pydantic#9794

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

- added support for URL path joining with optional trailing slashes and multiple arguments.
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.24%. Comparing base (ab503cb) to head (e8bd322).
Report is 185 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1378      +/-   ##
==========================================
- Coverage   90.21%   89.24%   -0.97%     
==========================================
  Files         106      112       +6     
  Lines       16339    17834    +1495     
  Branches       36       41       +5     
==========================================
+ Hits        14740    15916    +1176     
- Misses       1592     1898     +306     
- Partials        7       20      +13     
Files with missing lines Coverage Δ
src/url.rs 98.42% <100.00%> (+0.10%) ⬆️

... and 50 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

Copy link

codspeed-hq bot commented Jul 29, 2024

CodSpeed Performance Report

Merging #1378 will not alter performance

Comparing Meetesh-Saini:dev-url-join (e8bd322) with main (f389728)

Summary

✅ 155 untouched benchmarks

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The signature looks fine, though I think the implementation can be simpler.

src/url.rs Outdated Show resolved Hide resolved
src/url.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

I think this just needs test cases and then I would be happy to see this merged. Sorry for the long delay 😬

Meetesh-Saini and others added 2 commits September 28, 2024 01:25
- Refactor URL join function for better handling of relative paths
- Add tests for joining URLs with and without trailing slashes
- Cover various edge cases in line with URL specification that the previous function would fail to handle
@Meetesh-Saini
Copy link
Author

I've added tests and changed the implementation. Now, it only takes one argument instead of multiple.
I noticed that using multiple arguments can be confusing, especially when trailing_slash=False. Keeping it to one argument makes things clearer, similar to how servo-url and urllib.urljoin work.
For example, the user may expect either of the following. I wasn't sure which one to implement.

            a.join("e", "?q=1", "g", trailing_slash=False) 
                    │     │      │                         
                    └──┬──┘      │                         
                       ▼         │                         
              ┌───► "e?q=1"      │                         
              │        │         │                         
    No trailing slash  └────┬────┘                         
                            ▼                              
                           "g"                             
                                                       
                                                                                       
         a.join("e", "?q=1", "g", trailing_slash=False)
                 │     │      │                        
                 └──┬──┘      │                        
                    ▼         │                        
                 "e/?q=1"     │                        
                    │         │                        
                    └────┬────┘                        
                         ▼                             
                       "e/g"                           

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.

Request to add URL.join() to correctly join/construct new URLs
2 participants