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

Correct null handling when computing base URL host string #251

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

jeremyroman
Copy link
Collaborator

@jeremyroman jeremyroman commented Jan 14, 2025

@jeremyroman
Copy link
Collaborator Author

@shannonbooth Does this address the null handling issue you identified?

@jeremyroman
Copy link
Collaborator Author

It's maybe a little ambiguous which thing is being null checked, but the expansion into two separate points is also a little awkward. Still, can change to that if you think this wording is unclear.

shannonbooth

This comment was marked as duplicate.

@shannonbooth
Copy link

Oops, sorry, I somehow left that comment early. But yup, that fixes the issue I saw, thanks! (I'm getting somewhat close to finishing the implementation now, ~80% of the test suite passing :^) )

spec.bs Outdated
@@ -1858,8 +1858,7 @@ To <dfn>convert a modifier to a string</dfn> given a [=part/modifier=] |modifier
1. If |type| is not "`pattern`" and |init| [=map/contains=] none of "{{URLPatternInit/protocol}}", "{{URLPatternInit/hostname}}", "{{URLPatternInit/port}}" and "{{URLPatternInit/username}}", then set |result|["{{URLPatternInit/username}}"] to the result of [=processing a base URL string=] given |baseURL|'s [=url/username=] and |type|.
1. If |type| is not "`pattern`" and |init| [=map/contains=] none of "{{URLPatternInit/protocol}}", "{{URLPatternInit/hostname}}", "{{URLPatternInit/port}}", "{{URLPatternInit/username}}" and "{{URLPatternInit/password}}", then set |result|["{{URLPatternInit/password}}"] to the result of [=processing a base URL string=] given |baseURL|'s [=url/password=] and |type|.
1. If |init| [=map/contains=] neither "{{URLPatternInit/protocol}}" nor "{{URLPatternInit/hostname}}", then:
1. Let |baseHost| be the [=host serializer|serialization=] of |baseURL|'s [=url/host=].
1. If |baseHost| is null, then set |baseHost| to the empty string.
1. Let |baseHost| be the [=host serializer|serialization=] of |baseURL|'s [=url/host=], if it is not null, and the empty string otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Infra suggests:

Suggested change
1. Let |baseHost| be the [=host serializer|serialization=] of |baseURL|'s [=url/host=], if it is not null, and the empty string otherwise.
1. Let |baseHost| be the [=host serializer|serialization=] of |baseURL|'s [=url/host=], if it is not null; otherwise the empty string.

But I think initializing baseHost to the empty string and then conditionally overwriting it will be clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did the conditional overwrite thing.

@jeremyroman jeremyroman merged commit aa43064 into whatwg:main Jan 15, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants