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 grade badge for [lgtm] #1681

Merged
merged 5 commits into from
May 12, 2018
Merged

Add grade badge for [lgtm] #1681

merged 5 commits into from
May 12, 2018

Conversation

s0
Copy link
Contributor

@s0 s0 commented May 10, 2018

We've just launched project grades on lgtm: https://discuss.lgtm.com/t/announcing-project-grades-on-lgtm-com/1003

And this PR uses this data (that's exposed over the API) to add a new lgtm badge that displays the grade for a particular language.

EG:

rsyslog
rsyslog 1
cloudstack 2

Added new tests as appropriate.

@shields-ci
Copy link

shields-ci commented May 10, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @samlanning!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

@s0
Copy link
Contributor Author

s0 commented May 10, 2018

(Side note: I wasn't sure whether to include the language as part of the badge name or value, for consistency with the rest of the badges, i thought it would make more sense for it to also be in the value, but i'm open to suggestions)

@chris48s
Copy link
Member

Code looks fine to me - no comments there.

I think the way round you've proposed (with the language on the right) makes it seem like the colour is in some way connected to the language, whereas it is only the grade which influences the colour. Given that, maybe it would be better to go with

As a point of reference I think the closest comparison we already have is

screenshot at 2018-05-10 21 11 37

which doesn't include the service name in the badge. You might also consider something like .

Happy to defer to your opinion on whether you want to include the service name in the label or not.

@s0
Copy link
Contributor Author

s0 commented May 10, 2018

@chris48s i like your first solution actually, I've made those changes, also lower-cased the languages now that they're part of the label and that feels more consistent.

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.

👍 I think this is good to merge

@RedSparr0w
Copy link
Member

RedSparr0w commented May 11, 2018

I'm not quite sure as i've never used lgtm, But I feel like one of these may be better/more descriptive for the badge?




@s0
Copy link
Contributor Author

s0 commented May 11, 2018

Hmm, I see what you mean. Looking at the other badges that provide grades, it seems like "code quality" is the phrase to use here, so let's go with that. Dropping the service name probably also makes sense too. I like the code quality: python format.

I also realised i forgot to add the new badge to the homepage, so i've done that now.

@chris48s chris48s added the service-badge New or updated service badge label May 11, 2018
Copy link
Member

@RedSparr0w RedSparr0w 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 the changes, looks good!
👍

@chris48s chris48s merged commit 4f8414c into badges:master May 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants