yarl icon indicating copy to clipboard operation
yarl copied to clipboard

Add URL.joinpath(*elements, encoding=False) to build a URL with multiple new path elements

Open mjpieters opened this issue 2 years ago • 5 comments

What do these changes do?

This adds a new method, URL.joinpath(), which creates a new URL object with the arguments added as path elements:

>>> url = URL("http://example.com/foo")
>>> url.joinpath("bar", "ham", "spam/sub")
URL("http://example.com/foo/bar/ham/spam/sub")

The method accepts an encoded flag so you can pass in already-encoded path elements.

Are there changes in behavior for the user?

There are no changes in behaviour for existing methods, but I did refactor .__truediv__ to share code with the implementation for .joinpath().

Checklist

  • [x] I think the code is well written
  • [x] Unit tests for the changes exist
  • [x] Documentation reflects the changes
  • [x] Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

mjpieters avatar Mar 29 '22 14:03 mjpieters

Codecov Report

Merging #704 (dcced6d) into master (1a43a04) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head dcced6d differs from pull request most recent head 96afa11. Consider uploading reports for the commit 96afa11 to get more accurate results

@@           Coverage Diff           @@
##           master     #704   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files           4        4           
  Lines         750      750           
  Branches      170      212   +42     
=======================================
  Hits          748      748           
  Misses          2        2           
Flag Coverage Δ
unit 99.60% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
yarl/__init__.py 100.00% <100.00%> (ø)

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

codecov[bot] avatar Mar 29 '22 14:03 codecov[bot]

How different is this new method compared to using the following?

>>> url = URL("http://example.com/foo")
>>> url / "bar" / "ham" / "spam/sub"
URL('http://example.com/foo/bar/ham/spam/sub')

chey avatar Jun 11 '22 01:06 chey

How different is this new method compared to using the following?

You can't use that expression with a dynamic list of path elements, not without having to use an explicit loop. Without this method you can't extend a series of URLs in a list comprehension, say:

>>> import random
>>> from yarl import URL
>>> urls = [URL("http://example.com/foo"), URL("http://example.com/bar")]
>>> names = ("terry_jones", "eric_idle", "john_cleese", "micheal_palin", "graham_chapman", "terry_gilliam")
>>> [url.joinpath("spam", *random.choices(names, k=random.randint(1, 3))) for url in urls]
[URL("http://example.com/foo/spam/micheal_palin"), URL("http://example.com/bar/spam/eric_idle/terry_gilliam")]

This is modelled on pathlib.Path.joinpath(), where Path() objects of course also support / as a joining operator.

mjpieters avatar Jun 14 '22 11:06 mjpieters

@mjpieters I've rebased the PR and it still passes but there's still a few things I'd like improved so I won't be merging it right now.

webknjaz avatar Aug 01 '22 20:08 webknjaz

@webknjaz all comments addressed now.

mjpieters avatar Aug 08 '22 20:08 mjpieters

The pull request looks good but merge conflict should be resolved first.

asvetlov avatar Dec 13 '22 16:12 asvetlov

The pull request looks good but merge conflict should be resolved first.

Done! I'll merge it now that it's been green-lighted.

mjpieters avatar Dec 13 '22 23:12 mjpieters

Thanks @mjpieters !

asvetlov avatar Dec 14 '22 12:12 asvetlov