gh: release command improvements #481

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@calavera
Contributor

calavera commented Feb 10, 2014

Fixes #477

I've removed the defaults for the release command and now it will only upload assets explicitly. It detects whether you want to upload a file of every file inside a directory using the flag --attach or -a.

This change made all the tests that we had for this command obsolete. I'm trying to find a way we can add some coverage to the new changes.

Do not attach anything to the release command by default.
Detect the asset info to upload via the `--attach` flag.
@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Feb 10, 2014

Member

Great, thanks for working on this. Two things:

  1. What do you think about dropping the shorthand flag -a and just supporting --attach? I only support shorthand flags for often-typed commands, but I don't think the release create command will be typed often. I see it as something that will embedded as a part of a larger release script (since you need to build binaries first anyway).
  2. Our test suite on the "gh" branch is broken anyway (mostly Cucumbers) so you didn't need to comment out the tests. It's a bit unfortunate that a smaller change breaks all existing unit tests, though.
Member

mislav commented Feb 10, 2014

Great, thanks for working on this. Two things:

  1. What do you think about dropping the shorthand flag -a and just supporting --attach? I only support shorthand flags for often-typed commands, but I don't think the release create command will be typed often. I see it as something that will embedded as a part of a larger release script (since you need to build binaries first anyway).
  2. Our test suite on the "gh" branch is broken anyway (mostly Cucumbers) so you didn't need to comment out the tests. It's a bit unfortunate that a smaller change breaks all existing unit tests, though.
@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Feb 10, 2014

Contributor

What do you think about dropping the shorthand flag -a and just supporting --attach?

Personally, I like to have parity among flags. I always find strange when a command only offers shorthand flags for certain options.

It's a bit unfortunate that a smaller change breaks all existing unit tests, though.

Yeah, the problem is that we were testing the wrong thing here. The tests only covered how the assets were selected. Now that I've changed that all the tests are broken. I'd really like to have a mock http handler to test these kind of things.

Contributor

calavera commented Feb 10, 2014

What do you think about dropping the shorthand flag -a and just supporting --attach?

Personally, I like to have parity among flags. I always find strange when a command only offers shorthand flags for certain options.

It's a bit unfortunate that a smaller change breaks all existing unit tests, though.

Yeah, the problem is that we were testing the wrong thing here. The tests only covered how the assets were selected. Now that I've changed that all the tests are broken. I'd really like to have a mock http handler to test these kind of things.

@jingweno

This comment has been minimized.

Show comment
Hide comment
@jingweno

jingweno Feb 10, 2014

Collaborator

@calavera I've been using net/http/httptest for simulating the response in go-octokit. Here is one example: https://github.com/octokit/go-octokit/blob/master/octokit/client_test.go#L9-L28. Hope that helps.

Collaborator

jingweno commented Feb 10, 2014

@calavera I've been using net/http/httptest for simulating the response in go-octokit. Here is one example: https://github.com/octokit/go-octokit/blob/master/octokit/client_test.go#L9-L28. Hope that helps.

yasuoza added a commit to yasuoza/hub that referenced this pull request Sep 11, 2014

@jingweno jingweno closed this Oct 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment