URL.build(): Raise TypeError if host argument is None

[paulpapacz]

What do these changes do?

Make URL.build() raise a TypeError if host argument is None

Are there changes in behavior for the user?

No.

Related issue number

#808

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• 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.

[webknjaz]

FWIW I there are valid cases for not having a hostname. For example, this is how one would represent local filesystem paths or URLs, or UNIX file socket URLs, or abstract socket URLs. Hostnames are needed where a URI would represent an IP-network-accessible resource but a generic case would include things like http+unix:///var/tmp/aiohttp-server.sock even for web apps that are intended to operate locally. YARL is a generic lib so we shouldn't ignore use-cases that exist outside web. There's file:///etc/hosts or git+ssh://../another-repo/.git that are valid URLs. And there are URLs that are used in protocols targeting xdg-open/MIME thingies. Like steam://some-app?params or twitter://username etc.

If we want to force people pass a non-None host, we should at least make sure there are tests for these cases allowing the end-users to construct corresponding URLs with something like an empty string, for example.

[webknjaz]

But I guess having a None value representing “no hostname” seems to be better than an empty string that might have a different semantic meaning.
@Dreamsorcerer WDYT?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@ca96858). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 720efc9 differs from pull request most recent head 65fc161. Consider uploading reports for the commit 65fc161 to get more accurate results

@@            Coverage Diff            @@
##             master     #809   +/-   ##
=========================================
  Coverage          ?   99.34%           
=========================================
  Files             ?        4           
  Lines             ?      768           
  Branches          ?      176           
=========================================
  Hits              ?      763           
  Misses            ?        4           
  Partials          ?        1           

Flag	Coverage Δ
unit	99.34% <ø> (?)

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

Impacted Files	Coverage Δ
yarl/_url.py	99.66% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[paulpapacz]

@webknjaz - I agree with everything you said and I ran into this problem when I was trying to build a URL for a local file.

The thing is that the same argument could be made for path, query_string, fragment, etc but they are all handled this way (see block starting here).

In other words, with the changes here, the method simply becomes more consistent in the treatment of the various components.

My attempt here is to help people who run into the same issue to get a meaningful error message, rather than having to dig into the code.

[webknjaz]

@paulpapacz could you research if we would be able to have tests for those cases and maybe submit them first (even if we'll have to mark them as xfail for now)?

[webknjaz]

Oh, and one other case is relative URIs which only need path+qs.

[Dreamsorcerer]

But I guess having a None value representing “no hostname” seems to be better than an empty string that might have a different semantic meaning. @Dreamsorcerer WDYT?

Looks like the current situation is that it's not designed for it. At a glance, it looks much the same as the other parameters in that block, and fails if you try to use None:

>>> yarl.URL.build(host=None)  
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ubuntu/.local/lib/python3.10/site-packages/yarl/_url.py", line 246, in build
    cls._validate_authority_uri_abs_path(host=host, path=path)
  File "/home/ubuntu/.local/lib/python3.10/site-packages/yarl/_url.py", line 699, in _validate_authority_uri_abs_path
    if len(host) > 0 and len(path) > 0 and not path.startswith("/"):
TypeError: object of type 'NoneType' has no len()

I don't think there's any meaningful difference to "" or None representing "no hostname", the result would surely look the same, but would require extra code to convert the None.

So, this change looks fine to me. We should get type annotations added though, which would make it more explicit.

[Dreamsorcerer]

Nevermind, it uses stub files. This is already typed as str: https://github.com/aio-libs/yarl/blob/master/yarl/__init__.pyi#L59

[webknjaz]

@Dreamsorcerer looks like the CI needs to be fixed before merging this: #803

[webknjaz]

@Dreamsorcerer I'll leave the final merge to you if you don't mind.