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

Move PNG generation library code into a separate package and remove autodetection of dataDir #13

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sqs
Copy link

@sqs sqs commented Aug 29, 2013

This PR makes it possible to use buckler as a library.

Buckler is awesome, BTW!

@chadwhitacre
Copy link

@nathany Wanna review this PR?

@ghost ghost assigned nathany Dec 24, 2013
@nathany
Copy link
Contributor

nathany commented Dec 24, 2013

@sqs Can you look into why the Travis CI build is failing?

I can review this after the holidays. Thanks.

@sqs
Copy link
Author

sqs commented Dec 24, 2013

Will do

Sent from my iPhone

On Dec 24, 2013, at 12:46, Nathan Youngman notifications@github.com wrote:

@sqs Can you look into why the Travis CI build is failing?

I can review this after the holidays. Thanks.


Reply to this email directly or view it on GitHub.

@sqs
Copy link
Author

sqs commented Dec 31, 2013

I think the Travis CI build is failing because it's set up for jbowes/buckler, not gittip/img.shields.io. If that's not the proximate cause, then it makes it difficult to debug because clicking on the Details link above yields a page that just says "Loading."

I don't see this PR at https://travis-ci.org/gittip/img.shields.io/builds, but I think one of the repository owners could trigger a build of this PR over at that link. Then I will diagnose the issue.

@chadwhitacre
Copy link

@sqs I restarted the build using gittip/img.shields.io. The build did error, but the "Details" link here now gives more useful output.

@nathany
Copy link
Contributor

nathany commented Jan 3, 2014

#25 should help matters

@@ -11,11 +11,12 @@ import (
"strings"
"time"

"./shield"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe Go supports relative import paths. You'll need github.com/gittip/img.shields.io/shield.

@nathany
Copy link
Contributor

nathany commented Jan 3, 2014

@sqs I'm still brand new to this code base, but I'm not sure about deleting that resources.go code. Maybe resourcePaths() should still be used by main.go even if it's not used by the shield subpackage. Perhaps @jbowes can chime in on these changes.

@nathany
Copy link
Contributor

nathany commented Jan 3, 2014

@sqs #25 got Travis passing on master. It does introduce a small merge conflict (basePkg) for you to address. Thanks.

@sqs
Copy link
Author

sqs commented Jan 3, 2014

Great. Thanks for the CR and the other fixes. I'll push some changes to this PR to address these issues in a couple of hours.

@sqs
Copy link
Author

sqs commented Jan 5, 2014

OK, I pushed changes that address these comments and bring this branch up-to-date with master. Travis CI is now passing.

Regarding resourcePaths(), I personally find it simpler to be explicit about paths instead of guessing them according to convention. The old convention in resourcePaths() would only work if you run it with go run or if you copy the data/ and static/ dirs into your $GOBIN. I'm fine with either way, though, so let me know what you prefer. (Either way, it needs more changes; currently, staticDir is hard-coded and dataDir is specified via command-line option, and it only makes sense for both to be determined in the same way.)

@@ -12,6 +12,7 @@ import (
"time"

"github.com/droundy/goopt"
"github.com/gittip/img.shields.io/shield"
Copy link
Author

Choose a reason for hiding this comment

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

This will need to change per badges/shields#90 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having a shield subfolder, what do you think of flipping it around? Go stdlib and a few other places have a cmd folder.

so github.com/badges/buckler/cmd/buckler is the folder containing the buckler CLI.
so we could make github.com/badges/buckler the library import (stdlib actually has a pkg/ subfolder as well, but meh).

Also, let's just call this buckler (not shield).

Copy link
Author

Choose a reason for hiding this comment

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

sgtm!

@nathany
Copy link
Contributor

nathany commented Jan 7, 2014

Thanks @sqs.

Should there be some changes to the Command Line section of the README regarding resources?

We should also add a section on using it as a library, though it's going the be unstable for a bit as we rename repos and rearrange things again.

Hopefully @jbowes can take a look before we hit the big green button, just in case there's something we missed.

@sqs
Copy link
Author

sqs commented Jan 7, 2014

@nathany, yes, good point, we will need to update the README to describe the -d option, but I want to wait until it's decided whether we use -d or the previous discovery method (#13 (comment)). You or @jbowes can make the call. And I'll add library usage info to the README then, tooo.

@nathany
Copy link
Contributor

nathany commented Jan 7, 2014

@sqs Also check the discussion here. badges/shields#94 Everything's up in the air atm.

@nathany
Copy link
Contributor

nathany commented Jan 8, 2014

So I guess I'll do another bit of cleanup now that we've relocated once again.

@sqs
Copy link
Author

sqs commented Jan 8, 2014

Sounds like buckler won't be used by shields.io anymore, so this may be a moot point, right? (Re: badges/shields#90 (comment))

@nathany
Copy link
Contributor

nathany commented Jan 8, 2014

Yah. We'll see how that goes. What were your plans for the buckler library?

@sqs
Copy link
Author

sqs commented Jan 8, 2014

We've been using this branch for a few months to generate the badges at https://sourcegraph.com/help/authors/badges.

@nathany
Copy link
Contributor

nathany commented Jan 8, 2014

Ah, very cool. I've only looked at SourceGraph briefly, but was very impressed.

For now it makes sense to keep doing what you're doing. My only concern with changes here is that @jbowes should be able to deploy http://buckler.repl.ca/ and still have it work for all the API users (such as badges/shields#83).

Long term, it sounds like there will be two options:

  1. Make use of a web service API or redirect mechanism to the Node.js implementation once it ships/stable (in the same way Travis CI might use it).
  2. Continue using this alternate implementation, being careful to ensure consistency between all the badges on a page. I'm hoping we can get together a solid style guideline Document the colors supported by the custom Shield shields#74 which should help with that.

@nathany nathany mentioned this pull request Jan 9, 2014
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