Overload --json to fetch via PR number

[bytexenon]

Introduces the --pull-request PULL_REQUEST and -pr PULL_REQUEST flag. This flag accepts a pull request number, gets the head of the specified pull request, and fetches the latest data.json file from it.

Additionally, I've added a check to ensure that the --json and --pull-request flags are not used simultaneously. If both flags are used together, an error message will be displayed: "You cannot use both --json and --pull_request at the same time."

Example usage:

sherlock -pr 2331 bytexenon

sherlock --pull-request 2331 bytexenon

Feel free to leave any feedback or request changes as necessary.

Fixes #2354

[ppfeister]

Hm. Having some thoughts regarding UX.

@sdushantha any of your own?

Wondering if we should instead overload --json to take either a URI or (something like) a #0000 formatted PR ref, rather than adding a bespoke option.

i.e. --json #2356 and --json https[...]whateverUri.json each behaving differently according to the apparent intention.

If bespoke, I would probably drop the short -pr (multiple reasons) and leave only --pull-request.

Reference tagged issue for targeted functionality. Feel free to weigh in yourself if you have any thoughts @bytexenon

[ppfeister]

Regression tests ran at the time of this comment all PASS.

Failures indicated are due only to an existing positive test target (BodyBuilding[.]com) shutting down. This target will be swapped out elsewhere and can be disregarded.

tests/test_probes.py::TestLiveTargets::test_known_positives_via_response_url[BodyBuilding-blue] FAILED [ 44%]

[bytexenon]

Wondering if we should instead overload --json to take either a URI or (something like) a #0000 formatted PR ref, rather than adding a bespoke option.

i.e. --json #2356 and --json https[...]whateverUri.json each behaving differently according to the apparent intention.

It does sound much better than adding the --pull-request option, the only issue I have with it is the # symbol for PR numbers might require quotes around the argument (e.g., sherlock --json "#9876" bytexenon) because # is a comment character in Bash. Omitting the # and specifying just the number could make the command cleaner:

sherlock --json 9876 bytexenon

Ultimately, the decision is yours

[ppfeister]

Good catch. The # was just meant to better distinguish uri types (filepath, url, pull request), but I don't think it's really necessary. Let's do your version with just the number.

Think you can make it happen cleanly?

Additional notes for the current version. to be hopefully fixed in the new:

When passing a non-PR number (i.e. of an Issue or Discussion), it errors out with ERROR: 'head'. Can we add proper validation to see if the ref is actually a PR, and if not, print something more descriptive saying so

[bytexenon]

Think you can make it happen cleanly?

Of course, I'll work on it

When passing a non-PR number (i.e. of an Issue or Discussion), it errors out with ERROR: 'head'. Can we add proper validation to see if the ref is actually a PR, and if not, print something more descriptive saying so

Got it, thanks for pointing that out. I'll make sure to add proper validation and improve the error messages

[ppfeister]

Just confirming --- is this ready for review or are you looking to make some other tweaks? @bytexenon

[bytexenon]

Just confirming --- is this ready for review or are you looking to make some other tweaks? @bytexenon

I think it's done. Sorry for the late response, @ppfeister

[ppfeister]

No worries, just wanted to make sure it was actually done first -- didn't want to jump the gun

Changes look good and seem to function as expected. Merging!