Minor changes to url error messages [chromium/tools/depot_tools : main]
Jordan Brown (Gerrit)
Jordan Brown added 7 comments![]( "Open in Gerrit")
After some further thought, I've decided to move this check upstream to the autorollers, but I'd still like to land the minor improvement to the url error message.
f"Supported schemes: {sorted(list(_SUPPORTED_SCHEMES))}.")Jordan BrownDoes the hardcoded set really need to be sorted?
Done
# They must provide the fqdn e.g rpc://project.googlesource.com/project.Jordan Brown(optional) Is this documented anywhere? Would be good to have the README.chromium template or `adding_to_third_party.md` doc reflect this.
I've removed the rpc component of this change in favour of fixing it upstream in the autorollers.
if u.scheme == "rpc" and "." not in u.netloc:Jordan BrownDo you have a reference for this? I'm wary about relying on the presence of a character to determine whether a URL is a FQDN.
I've removed the rpc component of this change in favour of fixing it upstream in the autorollers.
Jordan Brownnit: not a fan of `validation_errors` not actually being `ValidationError`s. Maybe `error_messages`?
Done
validation_errors = set(filter(lambda x: x, map(_url_error, urls)))Jordan BrownWhat's the point of the `lambda` here? And `map`?
I think you can replace this with `set(filter(_url_error, urls))`.
That would return the original values, this needs to return the mapped error values. I just rewrote it to use a for loop so it's clearer.
additional=sorted(Jordan BrownIIRC you should be able to pass a set into `sorted`.
Done
Related details
- Anne Redulla
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anne Redulla (Gerrit)
Anne Redulla added 2 comments![]( "Open in Gerrit")
_SUPPORTED_SCHEMES = [The main check is an `in`, which is O(1) for sets. Arguably this should actually be a `frozenset`. Why is it important for the schemes to be deterministic in the error message?
def _url_error(url: str) -> Optional[str]:On more reflection, I think it's weird to name a function `_url_error`; later on, `_url_error(url)` reads like you are converting a url to an error. Strongly suggest to rename this to what the function aims to do.
Related details
- Jordan Brown
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |