Skip to content
This repository has been archived by the owner on Jun 20, 2022. It is now read-only.

Adding label endpoints #124

Merged
merged 14 commits into from
Dec 17, 2015
Merged

Adding label endpoints #124

merged 14 commits into from
Dec 17, 2015

Conversation

@half-ogre half-ogre changed the title Adding label endpoints [WIP] Adding label endpoints Nov 12, 2015
@half-ogre
Copy link
Contributor Author

@pengwynn: Do y'all have any interest in me adding end-to-end tests for the endpoints I'm adding that actually hit the API, and that are only run conditionally based on an environment variable?

@pengwynn
Copy link
Collaborator

@half-ogre Thanks for working through these. 💖 Would have gotten back sooner, but I was off the grid. ⛺

Do y'all have any interest in me adding end-to-end tests for the endpoints I'm adding that actually hit the API, and that are only run conditionally based on an environment variable?

Not quite sure I follow. It looks like you've wired up tests with the same pattern the other methods use, right?

@half-ogre
Copy link
Contributor Author

@pengwynn This PR is ready for review.

Not quite sure I follow. It looks like you've wired up tests with the same pattern the other methods use, right?

I was talking about adding acceptance tests in addition to the unit tests, that actually hit the live API, but the friction and amount of change this would add makes this not something to talk about in the context of this PR, even if you were interested. So, never mind. :)

@half-ogre half-ogre changed the title [WIP] Adding label endpoints Adding label endpoints Dec 3, 2015
@Tonkpils
Copy link
Contributor

👍 looking forward to these changes


assert.False(t, result.HasError())

assert.Equal(t, 2, len(labels))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some whitespace issues in some of the tests?

@pengwynn
Copy link
Collaborator

@half-ogre thanks for the patch! I saw some whitespace/alignment issues (maybe mix of tabs/spaces?). Once we fix those up this looks good. 👍

@half-ogre
Copy link
Contributor Author

I saw some whitespace/alignment issues (maybe mix of tabs/spaces?

My bad. I ran go fmt and all should be well now.

pengwynn added a commit that referenced this pull request Dec 17, 2015
@pengwynn pengwynn merged commit e0a883a into octokit:master Dec 17, 2015
@pengwynn
Copy link
Collaborator

🍰

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants