Skip to content

Conversation

watilde
Copy link
Contributor

@watilde watilde commented Jun 26, 2025

This change updates set_port so that when the input consists only of tab (\t), newline (\n), or carriage return characters, the port is left unchanged. Also adds these cases to ada_extra_setters_tests.json while waiting for their inclusion in WPT.

Refs: web-platform-tests/wpt#53420

@lemire
Copy link
Member

lemire commented Jun 26, 2025

My reading of the spec is as follows:

  1. If we set the empty string, the do port = std::nullopt;.
  2. Otherwise, try to parse the provided data as a 16-bit int, if this fails, return an error (and do not change the port).
  3. Otherwise set the port.

So I think @watilde is correct. Anything non-empty that does not parse to a 16-bit integer should leave the port unchanged.

Copy link
Member

@lemire lemire 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 it is a genuine bug worth fixing.

@lemire
Copy link
Member

lemire commented Jun 26, 2025

I'll wait for @anonrig to react before we merge.

@anonrig
Copy link
Member

anonrig commented Jun 27, 2025

It seems there are linting errors

@watilde
Copy link
Contributor Author

watilde commented Jun 27, 2025

Working on fixing the lint errors. I’ll push the changes shortly.

@watilde watilde force-pushed the fix/url-port-setter branch from b332e07 to 47b7cef Compare June 27, 2025 00:36
@watilde
Copy link
Contributor Author

watilde commented Jun 27, 2025

I ran clang-format to fix the lint errors, amended the previous commit, and force-pushed to keep the commit history clean.

@anonrig anonrig merged commit bd80521 into ada-url:main Jun 27, 2025
39 checks passed
@anonrig
Copy link
Member

anonrig commented Jun 27, 2025

Thank you for your contribution

@watilde watilde deleted the fix/url-port-setter branch June 27, 2025 06:31
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.

3 participants