Skip to content

Conversation

Master-Hash
Copy link
Contributor

Fix #27

@anonrig anonrig requested a review from bbayles September 2, 2023 12:07
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Looks good to me, although @bbayles probably has more knowledge on this than me.

@@ -290,7 +320,9 @@ def normalize_url(s):
return parse_url(s, attributes=('href',))['href']


def parse_url(s, attributes=PARSE_ATTRIBUTES):
# FIXME constrain `attributes``
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# FIXME constrain `attributes``
# FIXME constrain `attributes`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here - the docstring says unrecognized requests are ignored; we need not constrain with the type if we preserve that behavior.

@@ -354,7 +386,8 @@ def parse_url(s, attributes=PARSE_ATTRIBUTES):
return ret


def replace_url(s, **kwargs):
# FIXME constrain key of `**kwargs` by `URL_ATTRIBUTES`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docstring says that unknown keys are ignored; I'd like to preserve this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, but typing isn't just about error checking; constraints make auto suggestions work better.

If you insist, I'll just delete the comment; or I'll use Union[Literal["href"], ...].

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove the comments and create a GitHub issue to help make auto suggestions (what editor?) better.

@Master-Hash
Copy link
Contributor Author

Is it ready to merge?

@bbayles
Copy link
Collaborator

bbayles commented Sep 3, 2023

I'd like to add tests of the annotations with mypy, but we can do that in another PR.

@bbayles bbayles merged commit 3f2619e into ada-url:main Sep 3, 2023
@anonrig
Copy link
Member

anonrig commented Sep 3, 2023

🚀

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.

Type annotations
3 participants