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

github: Don't match a user unless the prefix is correct #147

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

jerry-skydio
Copy link
Collaborator

This fixes a longstanding annoying issue where github
queries users based on any match within username, whereas
generally people don't intend to use it this way.

When picking a valid user match, require the prefix to match
the stated query in addition to looking for the minimum size.

This fixes a longstanding annoying issue where github
queries users based on any match within username, whereas
generally people don't intend to use it this way.

When picking a valid user match, require the prefix to match
the stated query in addition to looking for the minimum size.
@jerry-skydio
Copy link
Collaborator Author

Reviews in this chain:
#147 github: Don't match a user unless the prefix is correct

@jerry-skydio
Copy link
Collaborator Author

jerry-skydio commented Mar 8, 2024

# head base diff date summary
0 4d70fb4f e43bfcfa diff Mar 7 16:16 PM 1 file changed, 3 insertions(+), 3 deletions(-)
1 0ea3dc42 9983f3e0 diff Mar 7 16:28 PM 1 file changed, 8 insertions(+)
2 5d71794d 9983f3e0 diff Mar 7 16:29 PM 1 file changed, 3 insertions(+), 1 deletion(-)

Copy link

@roy-vorster-skydio roy-vorster-skydio left a comment

Choose a reason for hiding this comment

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

🎉

Comment on lines 327 to 328
if this_node["totalCount"] > len(this_node["nodes"]):
logging.warning("Too many matching users found for {}".format(user_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this warn if there are multiple substring matches but only one prefix match?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this warning is for when github tells you there are more matches than it can return (max return is set to 25 atm). this would mainly happen if you there were more than 25 aarons, you wouldn't be able to match against a later alphabetical one without being more specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one warning that would be new that i'm not handling here is if none of the 25 returned names from github actually prefix match. in that case we would just take the first one rather than the shortest one. i think this is fine for now, but i'll add a warning for this case

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that seems fine

@jerry-skydio jerry-skydio force-pushed the jerry/revup/main/nomismatch branch 2 times, most recently from 0ea3dc4 to 5d71794 Compare March 8, 2024 00:30
@jerry-skydio jerry-skydio merged commit 0fc6b0e into main Mar 8, 2024
5 checks passed
@jerry-skydio jerry-skydio deleted the jerry/revup/main/nomismatch branch March 8, 2024 00:33
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.

4 participants