-
Notifications
You must be signed in to change notification settings - Fork 23
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
cmdget: accept package paths #63
base: master
Are you sure you want to change the base?
Conversation
Hey, @rogpeppe. No prob if you're busy, but it'd be nice to know what kind of response time I should expect on PRs and issues. It'll help me gauge how I want to slot in gohack work amongst my other obligations. (And might you also want to add Paul or Daniel as owners?) |
@josharian My github notifications are broken, it seems. Your reply there was the first email I got. Will review asap. |
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.
LGTM with one suggestion, and also I'd really like to see some tests if possible, please (ideally something to test the dedupe logic and what happens when a package isn't found).
Thanks for taking a look! (I don't suppose you want to also take a peek at the issues I filed here recently? :P) Will add tests and update. Currently all tests at master are failing for me, so I'll need to investigate first why. |
The test failures are due to rogpeppe/go-internal#84. |
(Still working on adding tests.) |
648c8af
to
63c43c9
Compare
Sigh. Testing this requires
I had hoped that using Go 1.12 would allow me not to have to invest time in goproxytest in order to make progress here. Nope. :P |
DO NOT MERGE The test fails because go-internal's proxy doesn't support .../@v/list requests. Fixes rogpeppe#62
63c43c9
to
6b292f3
Compare
I'd love to see this make it in |
Feel free to take it over! I’ve lost momentum and don’t have a pressing need for it any more. |
I’ll look into it. I opened a PR but the ci job didn’t seem to trigger..
…On Tue, Jul 27, 2021 at 5:24 PM Josh Bleecher Snyder < ***@***.***> wrote:
Feel free to take it over! I’ve lost momentum and don’t have a pressing
need for it any more.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#63 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAA7CJZ7QWMP7EUH2WUGD3TZ5E5RANCNFSM4I4224WA>
.
|
Fixes #62