Skip to content

cmdget: accept package paths#63

Open
josharian wants to merge 1 commit into
rogpeppe:masterfrom
josharian:get-by-package-path
Open

cmdget: accept package paths#63
josharian wants to merge 1 commit into
rogpeppe:masterfrom
josharian:get-by-package-path

Conversation

@josharian
Copy link
Copy Markdown
Contributor

@josharian josharian commented Oct 2, 2019

Fixes #62

@josharian
Copy link
Copy Markdown
Contributor Author

josharian commented Oct 7, 2019

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
Copy Markdown
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.

rogpeppe
rogpeppe requested changes Oct 7, 2019
View reviewed changes
Copy link
Copy Markdown
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).

Comment thread cmdget.go Outdated
@josharian
Copy link
Copy Markdown
Contributor Author

josharian commented Oct 7, 2019

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
Copy Markdown
Contributor Author

josharian commented Oct 7, 2019

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

@josharian
Copy link
Copy Markdown
Contributor Author

josharian commented Oct 7, 2019 •
edited
Loading

(Still working on adding tests.)

@josharian josharian force-pushed the get-by-package-path branch from 648c8af to 63c43c9 Compare October 7, 2019 23:40
@josharian
Copy link
Copy Markdown
Contributor Author

josharian commented Oct 7, 2019

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

@josharian
cmdget: accept package paths
6b292f3 
DO NOT MERGE

The test fails because go-internal's proxy
doesn't support .../@v/list requests.

Fixes rogpeppe#62
@josharian josharian force-pushed the get-by-package-path branch from 63c43c9 to 6b292f3 Compare October 8, 2019 16:17
@tmc
Copy link
Copy Markdown

tmc commented Jul 27, 2021

I'd love to see this make it in

@josharian
Copy link
Copy Markdown
Contributor Author

josharian commented Jul 28, 2021

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

@tmc
Copy link
Copy Markdown

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

Reviewers

@rogpeppe rogpeppe rogpeppe requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

accept package path in 'gohack get'

3 participants

@josharian @rogpeppe @tmc