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

Align label and message props of coalesceBadge() with internal makeBadge() #5719

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Oct 16, 2020

There is an internal makeBadge() function which is called from a few places in the server and from the public makeBadge() function which is a light wrapper. (Eventually we want to dogfood the public API: that's the work of #4950, and this helps with it by aligning the interfaces.)

Related to that is #3370, which is about aligning the serviceData schema (i.e. the result of handle()) with the public makeBadge() function.

A legacy quirk of the private makeBadge() function is accepting a text: ['label', 'message'] array instead of separate { label, message } props like the rest of the codebase. coalesceBadge() has to translate from { label, message } to text: ['label', message']. This removes that bit of indirection.

It also rewrites most of the tests of coalesceBadge() to use .includes(), providing IMO a slight improvement in readability.

This is cherry-picked from #4980 but stops short of the more substantive (and maybe controversial) changes of that PR.

…`makeBadge()`

Part of #3370, and also helps with #4950.

This is cherry-picked from #4980 but stops short of the mor substantive changes of that PR.

Also rewrites most of the tests of `coalesceBadge()` to use `.includes()`, providing IMO a slight improvement in readability.
@paulmelnikow paulmelnikow added core Server, BaseService, GitHub auth, Shared helpers npm-package Badge generation and badge templates labels Oct 16, 2020
@shields-cd shields-cd temporarily deployed to shields-staging-pr-5719 October 16, 2020 03:54 Inactive
@shields-ci
Copy link

shields-ci commented Oct 16, 2020

Warnings
⚠️ This PR modified the server but none of its tests.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS against 064c198

@@ -6,10 +6,6 @@ const Joi = require('joi')
const makeBadge = require('../../badge-maker/lib/make-badge')
const BaseSvgScrapingService = require('./base-svg-scraping')

function makeExampleSvg({ label, message }) {
return makeBadge({ text: ['this is the label', 'this is the result!'] })
}
Copy link
Member Author

@paulmelnikow paulmelnikow Oct 16, 2020

Choose a reason for hiding this comment

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

🙈

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-5719 October 16, 2020 04:14 Inactive
@paulmelnikow paulmelnikow force-pushed the text-to-label-message branch from c8e0c4f to 356a9a4 Compare October 16, 2020 04:16
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.

LGTM 👍

@github-actions
Copy link
Contributor

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:

paulmelnikow added a commit that referenced this pull request Oct 16, 2020
This is an analogous change to #5719 for `’template’` and `’style’`. (See the top comment for more explanation.)

The diff is simpler since there are fewer test changes in this one.
paulmelnikow added a commit that referenced this pull request Oct 20, 2020
…5726)

This is an analogous change to #5719 for `’template’` and `’style’`. (See the top comment for more explanation.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers npm-package Badge generation and badge templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants