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

Update URL logic parsing to not force xethub.com as host #128

Merged
merged 6 commits into from
May 6, 2024

Conversation

hoytak
Copy link

@hoytak hoytak commented May 3, 2024

  • Fixes bugs in parsing (parsing tests were never actually run)
  • Tests now pass.
  • Explicit domains are not forced to be xethub.com.
  • Deprecation warnings given now.

@hoytak hoytak requested a review from seanses May 3, 2024 21:58
else:
raise ValueError(f"Cannot parse user and endpoint from {parse.netloc}")

if not parse.netloc.endswith(".com"): # Cheap way now to see if it's a website or not; we won't hit this with the old format.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we have a xethub.uw.edu deployment in the future, hmmm, just a guess

Copy link
Author

Choose a reason for hiding this comment

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

This is only on the non-@ path with will be deprecated, so I'm not worried about future stuff. This is just to see if the start is xet://domain/user/... or xet://user/ You can break this by having a user that ends in .com, but I'm also not worried about that now...

Copy link
Contributor

@seanses seanses left a comment

Choose a reason for hiding this comment

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

LGTM!

@hoytak hoytak merged commit 82cf70c into main May 6, 2024
3 checks passed
@hoytak hoytak deleted the hoytak/240502-url-parsing branch May 6, 2024 22:17
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.

2 participants