- Notifications
You must be signed in to change notification settings - Fork825
chore(utils): relax GitHub PAT validation (#380)#383
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
* Drop `InvalidGitHubTokenError`* `validate_github_token` now emits a `UserWarning` instead of raising* Update tests to: * assert no warnings for valid tokens (via `recwarn`) * expect `UserWarning` for malformed tokens* Reduces false-positives and is future-proof if GitHub introduces new token formatsCloses#380
| """ | ||
| if not re.fullmatch(_GITHUB_PAT_PATTERN, token): | ||
| raise InvalidGitHubTokenError | ||
| warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We don't need to validate the token with pattern matching and the warning is useless here i guess.
When the token is invalid (malformed or just invalid) we correctly handle the error:
I don't see why we validate and log the validation result here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the validation is informative. Just a little message saying "this does not look like a proper token, tokens usually start with gh_ blablabla" because yeah, believe me, i've seen too many people struggle with access tokens and just put their plain password or something
InvalidGitHubTokenErrorvalidate_github_tokennow emits aUserWarninginstead of raisingrecwarn)UserWarningfor malformed tokensCloses#380