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

Improve robustness of GitHub repo URL parsing #71

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

devnoname120
Copy link
Contributor

@devnoname120 devnoname120 commented Nov 18, 2024

For example this works now: https://github.com/MichaelAquilina/zsh-auto-notify/pull/67/files
Previously 67 was taken as the repo owner and files as the repo name.

For example this works now: https://github.com/MichaelAquilina/zsh-auto-notify/pull/67/files
Previously `67` was taken as the repo owner and `files` as the repo name.
Copy link
Collaborator

@payne911 payne911 left a comment

Choose a reason for hiding this comment

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

Thanks for this! It's a neat improvement.

A few things to change :)
After your changes, please provide a screenshot showing the results of different test-cases:

  • empty input
  • bob
  • bob/bob
  • https://github.com/useful-forks/useful-forks.github.io/pull/71/files
  • useful-forks/useful-forks.github.io/pull/71/files
  • github.com/bob
  • github.com/payne911/PieMenu
  • payne911/PieMenu

Example
image

website/src/queries-init.js Outdated Show resolved Hide resolved
website/src/queries-init.js Outdated Show resolved Hide resolved
website/src/queries-init.js Show resolved Hide resolved
@devnoname120
Copy link
Contributor Author

@payne911 I don't have the time to take 8 screenshots but here are the results:

  • empty input

Performs a search for payne911/PieMenu.

  • bob
Please enter a valid query: it should contain two strings separated by a "/"
  • bob/bob
There seems to have been an error.
Maybe you had a typo in the provided input? Or the Access Token credentials are invalid?
If the scan is continuing, ignore this: the GitHub API some times returns erroneous data.
  • https://github.com/useful-forks/useful-forks.github.io/pull/71/files

The query field content is replaced with useful-forks/useful-forks.github.io and it performs that query.

  • useful-forks/useful-forks.github.io/pull/71/files
Please enter a valid query: it should contain two strings separated by a "/", or the full URL to a GitHub repo
  • github.com/bob
There seems to have been an error.
Maybe you had a typo in the provided input? Or the Access Token credentials are invalid?
If the scan is continuing, ignore this: the GitHub API some times returns erroneous data.
  • github.com/payne911/PieMenu
Please enter a valid query: it should contain two strings separated by a "/", or the full URL to a GitHub repo
  • payne911/PieMenu

Performs a search for payne911/PieMenu.


If you have further comments and in order to avoid back-and-forths, I'd be grateful if you could address them directly. 🙏 Note that you have write access to this branch.

@devnoname120 devnoname120 requested a review from payne911 December 3, 2024 23:18
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