Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Leanpub Book Summary Badges [LeanpubBookSummary] #2458

Merged
merged 8 commits into from
Dec 6, 2018

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Dec 4, 2018

Adds new service to provide badges for Leanpub book metrics which closes #2270

Comments:

  • I wasn't sure what category this would fall under, so I chose other
  • One of the badges (revenue) will only be available on self-hosted Shields instances. I added a note to the docs for that badge to indicate this, but not sure if there are other conventions/standards followed for self-hosted only badges
  • The Leanpub API docs all reference providing the API Key (which would only be done for self-hosted Shields instances) via a query param. I'd prefer that be done via auth headers, but figured I'd just go with what their docs say, especially since I don't personally use the service

@calebcartwright calebcartwright changed the title Add Leanpub Book Summary Badges Add Leanpub Book Summary Badges [LeanpubBookSummary] Dec 4, 2018
@shields-ci
Copy link

Messages
📖 ✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 5, 2018
@chris48s
Copy link
Member

chris48s commented Dec 6, 2018

Thanks for submitting this.

I'm not sure what to do about the revenue badge. I don't think its particularly useful to add/document a badge on shields.io that nobody can use. Does anyone know of another example where we've got a badge in core that only works on a self-hosted instance?

@paulmelnikow
Copy link
Member

I'm not sure what to do about the revenue badge. I don't think its particularly useful to add/document a badge on shields.io that nobody can use. Does anyone know of another example where we've got a badge in core that only works on a self-hosted instance?

I'm not aware of any.

In theory I wouldn't mind maintaining a badge that targets self-hosing users, though I'm not sure if there is anyone who wants the revenue badge enough to self-host.

Given that, maybe we could leave off the revenue badge for now?

@calebcartwright
Copy link
Member Author

I think that's fair, and also aligns with what the original requestor said when we found it out would only be available for self-hosted.

I'll remove the revenue badge. Any other changes needed?

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2458 December 6, 2018 20:05 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2458 December 6, 2018 20:14 Inactive
@chris48s
Copy link
Member

chris48s commented Dec 6, 2018

cheers. I've left a couple of other minor comments to follow up on

@calebcartwright
Copy link
Member Author

Also a semi-random question. I noticed in the code coverage results that the examples method is the only one that doesn't get hit (the other static methods like category and defaultBadgeData do get hit) resulting in coverage < 100%. Am I missing something in my tests or is this the expected behavior?

@chris48s
Copy link
Member

chris48s commented Dec 6, 2018

Good question. I think if you run the full test suite (as opposed to just the service tests) with npm test there is a test in there somewhere which builds the full front-end data structure. That has the effect of touching all the service examples which will cause it to fail if something in there throws an exception, but we don't really test the examples beyond that.

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

thanks for you work on this

@chris48s chris48s merged commit b2d5d3e into badges:master Dec 6, 2018
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

calebcartwright added a commit to calebcartwright/shields that referenced this pull request Dec 6, 2018
Add Leanpub Book Summary Badges [LeanpubBookSummary] (badges#2458)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LeanPub Badges
4 participants