Skip to content

Conversation

@wking
Copy link

@wking wking commented Jun 4, 2014

I'd like to centralize commit formatting between Hubot scripts, and
this seemed like an appropriate place. I've stubbed this out, but the
tests are currently failing and I haven't had time to track down the
reasons yet (assistance welcome!). Once this gets fixed up and
released, we can use it in github/hubot-scripts#1461.

@wking wking changed the title WIP: Add commit formatting etc. Add commit formatting (optional oneline and gitio reduction) Jun 7, 2014
@wking
Copy link
Author

wking commented Jun 7, 2014

Ok, with 72a05c5 I'm passing local tests, so I think this is ready for review and merging.

@wking
Copy link
Author

wking commented Jun 10, 2014

The Travis errors appear to be due to incompatibility with older versions of Node. For example:

npm ERR! Error: No compatible version found: debug@'^0.8.1'
npm ERR! Valid install targets:
npm ERR! ["0.0.1","0.1.0","0.2.0","0.3.0","0.4.0","0.4.1","0.5.0","0.6.0","0.7.0","0.7.1","0.7.2","0.7.3","0.7.4","0.8.0","0.8.1","1.0.0","1.0.1"]

I didn't mess with any of that, so I expect the current master has the same failures independent of this pull request.

@parkr
Copy link

parkr commented Jun 10, 2014

Docs would also be nice for this.

@wking
Copy link
Author

wking commented Jun 10, 2014

On Tue, Jun 10, 2014 at 11:46:12AM -0700, Parker Moore wrote:

Docs would also be nice for this.

Readme entries?

@parkr
Copy link

parkr commented Jun 10, 2014

Readme entries?

Yurp. Adding the env vars, possible values, and what they do when set to each of those values.

@wking
Copy link
Author

wking commented Jun 10, 2014

Hmm, actually, I did add the gitio2 dependency. But the debug package
is only being pulled in by nock (v0.32.3 requires debug ^0.8.1) and
mocha (v1.20.0 required debug *).

@wking
Copy link
Author

wking commented Jun 10, 2014

On Tue, Jun 10, 2014 at 11:52:11AM -0700, Parker Moore wrote:

Adding the env vars, possible values, and what they do when set to
each of those values.

Writing these up turned up a number of issues (e.g. commit.html_url is
only set for one endpoint). But with 3288596 I think I've addressed
all of that. Let me know if anything is unclear ;).

@iangreenleaf
Copy link
Owner

I'm undecided on whether I want to merge this or not. It's cool functionality and useful, but it perhaps strays a little bit from githubot's core mission.

At any rate, the test failures aren't your fault, something must have changed in the dependencies since the last time master ran on Travis. I re-ran it and master is erroring now too.

@iangreenleaf
Copy link
Owner

Ok, fixed those errors in master, so if you merge, this branch should go green too.

@wking
Copy link
Author

wking commented Jun 10, 2014

On Tue, Jun 10, 2014 at 04:12:00PM -0700, Ian Young wrote:

I'm undecided on whether I want to merge this or not. It's cool
functionality and useful, but it perhaps strays a little bit from
githubot's core mission.

Sure. I was looking for a lower-level place to put code shared
between hubot-scripts' github-commits and github-commit-links (and
possibly other scripts) to avoid copy-pasting the formatting code (as
I'm doing in the current round of github/hubot-scripts#1461).
githubot seemed like a good place to store code for processing GitHub
API results, but this code could also live in a stand-alone
github-commit-formatting package, or some such.

The formatting code is pretty small for a stand-alone package, so I'd
rather include it in githubot. That makes life easier for consumers
too (only one dependency), and for me (no need to recreate a bunch of
packaging boilerplate). But if you feel that it's adding too much of
a maintenance burden, I'll suck it up and create a new package ;).

Also add a few GitHub API wrappers, to mirror gh.branches.  To
distinguish between the API wrappers and the formatter, we use the
presence of a callback in the initial call.  So:

  gh.commits(repo, commit, cb)

takes a repo name, commit hash, and callback; fetches a commit from
GitHub; and calls the callback on the returned commit.  On the other
hand,

  gh.commits().format(commit, cb)

takes a commit object (as returned from GitHub) and callback, and
calls the callback on the formatted commit.

The options formatting depends on two options, as discussed in the
Readme:

* gitio (set from HUBOT_GITIO) toggles git.io URL shortening
* oneline (set from HUBOT_COMMIT_ONELINE) toggles message reduction to
  just the summary line.

The bulk of the logic was ported from hubot-scripts [1,2], but I've
adjusted the URL munging.  When it's set [3], I leave commit.html_url
alone.  For the other endpoints [4,5], I try to parse the API URL and
convert it in a single pass.  I don't have access to an enterprise
instance for testing [6], but I'm assuming the API URLs look like:

  :apiRoot/api/:apiVersion/repos/:owner/:repo/commits/:sha
  https://yourdomain.com/api/v3/repos/:owner/:repo/commits/:sha

and the HTML URLs looks like:

  :apiRoot/repos/:owner/:repo/commit/:sha
  https://yourdomain.com/repos/:owner/:repo/commit/:sha

[1]: https://github.com/github/hubot-scripts/blob/v2.5.14/src/scripts/github-commit-link.coffee
[2]: https://github.com/github/hubot-scripts/blob/v2.5.14/src/scripts/github-commits.coffee
[3]: https://developer.github.com/v3/repos/commits/#get-a-single-commit
[4]: https://developer.github.com/v3/git/commits/#get-a-commit
[5]: https://developer.github.com/v3/activity/events/types/#pushevent
[6]: https://developer.github.com/v3/#schema
@wking
Copy link
Author

wking commented Jun 10, 2014

On Tue, Jun 10, 2014 at 04:33:42PM -0700, Ian Young wrote:

Ok, fixed those errors in master, so if you merge, this branch
should go green too.

Rebased and force-pushed.

@wking
Copy link
Author

wking commented Aug 8, 2014

On Tue, Jun 10, 2014 at 04:36:32PM -0700, W. Trevor King wrote:

Rebased and force-pushed.

Ping.

@wking
Copy link
Author

wking commented Oct 8, 2014

On Tue, Jun 10, 2014 at 04:36:32PM -0700, W. Trevor King wrote:

Rebased and force-pushed.

Ping again ;).

@iangreenleaf
Copy link
Owner

Thanks for the ping - this had definitely slipped a long ways down my inbox. I've come back around and now agree that this seems reasonable for inclusion into githubot. Since git.io is an official GitHub service, there's a certain logic to offering it as a built-in dependency here. So yeah, let's do it! Thanks for writing tests, that helps a ton.

One small suggestion: I'm thinking we could change the structure of these calls ever so slightly to make them stylistically consistent with the existing stuff (for ex, the deployments API), give it a bit more fluent of a feel, and separate the different bits of functionality. Here's what I'm thinking it could look like:

gh.commits "iangreenleaf/githubot", (commits) -> # The index action

gh.commits("iangreenleaf/githubot").details "01234567deadbeef", (commit) -> # Single commit

gh.commits.format commit, (result) -> # The standalone formatting - yes, I intended to omit the parens

Does that make sense?

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.

3 participants