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

cmdget: accept package paths #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josharian
Copy link
Contributor

Fixes #62

@josharian
Copy link
Contributor Author

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?)

@rogpeppe
Copy link
Owner

rogpeppe commented Oct 7, 2019

@josharian My github notifications are broken, it seems. Your reply there was the first email I got. Will review asap.

Copy link
Owner

@rogpeppe rogpeppe left a 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).

cmdget.go Outdated Show resolved Hide resolved
@josharian
Copy link
Contributor Author

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.

@josharian
Copy link
Contributor Author

The test failures are due to rogpeppe/go-internal#84.

@josharian
Copy link
Contributor Author

josharian commented Oct 7, 2019

(Still working on adding tests.)

@josharian
Copy link
Contributor Author

Sigh. Testing this requires list support from goproxytest:

            go get golang.org/x/text: unexpected status (http://127.0.0.1:61947/mod/golang.org/x/text/@v/list): 404 Not Found

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
@tmc
Copy link

tmc commented Jul 27, 2021

I'd love to see this make it in

@josharian
Copy link
Contributor Author

Feel free to take it over! I’ve lost momentum and don’t have a pressing need for it any more.

@tmc
Copy link

tmc commented Jul 28, 2021 via email

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.

accept package path in 'gohack get'
3 participants