[CodeClimate] Fix remaining badges

[PyvesB]

Hello there,

This pull request is related to discussions in #1368 and #1329. @RedSparr0w would probably be the ideal person to review this as he has been involved in the discussions and the previous pull request. 😉

We should now have five functional Code Climate badges, with tests:

• /codeclimate/c: coverage score (letter).
• /codeclimate/coverage: coverage percentage.
• /codeclimate: technical debt percentage (related to the maintainability of the project).
• /codeclimate/maintainability: maintainability score (letter).
• /codeclimate/issues: number of issues.

Nevertheless, I'm still unhappy with the current state of our code for several reasons:

• the distinction between /codeclimate/coverage and /codeclimate/c is confusing for both users and developers. One is a percentage, the other is just a letter related to the percentage. /codeclimate/coverage_score and /codeclimate/coverage_percentage would have been more consistent in my opinion.
• the URL of the technical debt badge suggests this is a top level metric. This is not the case. Similarly to the two test coverage badges, the technical debt is a percentage, /codeclimate/maintainability is just a letter related to the percentage. The two URLs should be along the lines of /codeclimate/maintainability_score and /codeclimate/maintainability_percentage. Previously /codeclimate probably corresponded to the global GPA metric, which was deprecated in favour of two top-level ratings, maintainability and test coverage (see blog post). In addition to the naming inconsistency, we have something that looks top-level, but actually isn't.

Unfortunately, I don't see how to resolve these problems without impacting users of the existing badge URLs. My approach in this PR was to match the 5 existing shields URLs to metrics that seemed to reflect as closely as possible the old badges, and make sure we have tests for all badges.

As a side note, there is a bit of code duplication in server.js (~15-20 lines), as both the badges who call the snapshots and test_reports endpoints have to make the same first call to the repos parent endpoint. Merging the two was too messy, so I broke things down and hope that we can get rid of the code duplication nicely once we have neat separate services.

Cheers,

Pyves

[shields-ci]

	Messages
📖	✨ Thanks for your contribution to Shields, @PyvesB!
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

[RedSparr0w]

Yeah i get where you are coming from,
What do you think of making /coverage behave exactly the same as /c,
Then add another part to the regex (\/percentage)? which would instead show a percentage rather than the default of A-F when included?
Also the same with /maintainability
so we would have:

endpoint	example badge
/c
/c/percentage
/coverage
/coverage/percentage
/maintainability
/maintainability/percentage
/issues

Not sure what to do about the /codeclimate top level badge, I don't think it would be the worst thing to drop support for it, but i'm also not sure how used it is.
Do you possibly have any stats @espadrine & @paulmelnikow or any other ideas in regards to this?

You may be able to merge them all/remove some of the duplication with a switch case similiar to this possibly?

[paulmelnikow]

I haven't dived into this too closely, though I could offer some quick thoughts on the API design:

• A lot of these CodeClimate badges have been broken for a long time, which means a lot of users may be newly setting these up. That means they'll be referring to our website to find the syntax. So let's focus on designing a new API that we like and getting users onto it over time.
• Let's make the old stuff work in a backward-compatible way whenever possible… and ideally pick a new API that doesn't conflict with the old stuff. 😄
• We're using /c on a number of badges for coverage percentage, and I think ideally we'd do the same here. (The /c badge on the production site is a number, so maybe that is already happening?)
• I'd rather use dashes than slashes or underscores to connect other options – e.g. /c-score. Or even /c-letter. Score is ambiguous: either a percentage or a letter grade could be a score. Letter this seems closer to the terms used in Code Climate's documentation.
• We don't have stats by service or route (Record stats by service #966).
• In the long run, we should think about designing an approach to badge deprecation. For example, we could launch the new badge, then a little while later add a deprecation warning to the old badge, and a while after that redirect to documentation or turn it off entirely.

[RedSparr0w]

• A lot of these CodeClimate badges have been broken for a long time, which means a lot of users may be newly setting these up. That means they'll be referring to our website to find the syntax. So let's focus on designing a new API that we like and getting users onto it over time.
• Let's make the old stuff work in a backward-compatible way whenever possible… and ideally pick a new API that doesn't conflict with the old stuff.

In terms of keeping backwards compatibility for the base /codeclimate,
we could possibly change this line to something like /^\/codeclimate\/(c|coverage)? making the c|coverage optional and should still return the coverage.

• We're using /c on a number of badges for coverage percentage, and I think ideally we'd do the same here. (The /c badge on the production site is a number, so maybe that is already happening?)

From what i can gather, the /c badge used to originally return a letter (#1196, also a fairly new badge), but once the API and other features were introduced it started showing a number which in turn caused issue #1329

It does look like percentage may be the main scoring format for coverage though:
And letter for maintainability:

• I'd rather use dashes than slashes or underscores to connect other options – e.g. /c-score. Or even /c-letter. Score is ambiguous: either a percentage or a letter grade could be a score. Letter this seems closer to the terms used in Code Climate's documentation.

Now that you mention it, when looking through the current badges, dashes do seem to be commonly used, specifically quite a lot in GitHub badges.

• In the long run, we should think about designing an approach to badge deprecation. For example, we could launch the new badge, then a little while later add a deprecation warning to the old badge, and a while after that redirect to documentation or turn it off entirely.

Maybe a symbol at the end or logo at the beginning for soon to be depreciated badges,
then change the badge to or similar
before completely removing it, returning

[PyvesB]

Okay, I believe this is now ready for review. Here are the available badges:

Endpoint	Badge
(top-level)
/c
/c-letter
/coverage
/coverage-letter
/maintainability
/maintainability-letter
/issues

The /maintainability badge above is still subject to discussion. The Code Climate API related to maintainability returns a technical debt ratio value, which is supposed to be low for maintainable projects. To be consistent with the badge URL, we could calculate a new metric, which would simply be:
maintainability = 100 - technical debt
This would lead to the following badge: . Any thoughts?

As far as the deprecation of badges is concerned, more discussions are probably necessary and there may be other service badges involved. So far, I have just removed the top-level and /coverage badges from the homepage examples, and added a comment in the code explaining that they are supported for backwards compatibility only.

[RedSparr0w]

Everything is looking good so far.

Not sure what should be done about the /maintainability badge,
IMO it looks odd having the percentage as the default,
Maybe it could use -percentage or just omit percentage completely.
Though i do like your idea of doing maintainability = 100 - technical debt

And as the codes already there add tech debt as a new option /tech-debt /tech_debt /technical-debt /technical_debt.
currently the only other badge with tech debt is sonarqube which uses tech_debt, but it also seems to be the only badge using _ > -

Any thoughts @paulmelnikow ?

[RedSparr0w]

Thanks for all your work on this,
All the changes look great 😄

🎉 Merged!

[PyvesB]

Glad this got merged, it was a non trivial piece of work! Hopefully the new Code Climate badges won't break any time soon! 😃

[timkurvers]

This is great, thanks @PyvesB and @RedSparr0w!

This may be a silly question, but how long does it usually take for these type of changes to hit production?

[platan]

Hi @timkurvers, I will quote @paulmelnikow on that:

Deploys usually happen every 1–3 weeks. Thaddée, who has limited time on this project, is the only sysadmin. He's working on giving me access to deploy and logs, but doing so is complicated because the hosting account (and maybe the servers too) are shared with other services he runs.

[platan]

@PyvesB after 435903a service tests of codeclimate crashes. CI https://circleci.com/gh/badges/shields/1880, locally I have similar result

> node_modules/mocha/bin/_mocha --delay service-tests/runner/cli.js --only=codeclimate
/home/m/workspaces/github/shields-platan/node_modules/joi/lib/types/object/index.js:362
                throw castErr;
                ^

Error: Invalid schema content: 
    at Object.exports.assert (/home/m/workspaces/github/shields-platan/node_modules/hoek/lib/index.js:736:11)
    at Object.exports.schema (/home/m/workspaces/github/shields-platan/node_modules/joi/lib/cast.js:55:10)
    at keys (/home/m/workspaces/github/shields-platan/node_modules/joi/lib/types/object/index.js:352:35)
    at Object.<anonymous> (/home/m/workspaces/github/shields-platan/service-tests/codeclimate.js:14:33)
...

[PyvesB]

@platan: The tests are running fine on the branch. This pull request was submitted back in December, so it's probably another pull request that got merged in the meantime that indirectly broke them. I'll have a look into the issue this week-end. 😉

[RedSparr0w]

@platan should be fixed with #1482

Seems the validator that was being used changed since this PR was opened.

[platan]

You spotted source of the problem quite fast.
Merge with the master before merging this PR probably would help in this case.