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.

Sign up for GitHub

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

Jump to bottom

Migrate to Express [*****] #7877

Closed
wants to merge 25 commits into from
Closed

Migrate to Express [*****] #7877

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6d72fd6
Begin to replace scoutcamp with express
paulmelnikow Apr 17, 2022
7c071e3
Core tests passing
paulmelnikow Apr 17, 2022
6e6cec9
Move one test
paulmelnikow Apr 17, 2022
c3097aa
Fix / remove obsolete tests
paulmelnikow Apr 17, 2022
aeebfaa
Renames
paulmelnikow Apr 17, 2022
cf6b5b1
Remove obsolete code
paulmelnikow Apr 17, 2022
15043df
Remove more obsolete code
paulmelnikow Apr 17, 2022
903bef2
Finish rename
paulmelnikow Apr 17, 2022
aa046cb
Fixes
paulmelnikow Apr 17, 2022
be9d490
More tests passing
paulmelnikow Apr 17, 2022
8cbc8cb
More tests passing
paulmelnikow Apr 17, 2022
e8f59a2
More fixes
paulmelnikow Apr 17, 2022
6d77534
Progress / cleanup
paulmelnikow Apr 22, 2022
b4f7ec3
Update integration tests
paulmelnikow Apr 22, 2022
00df3e1
Clean diff
paulmelnikow Apr 22, 2022
b19ca20
Update docs and suggest test
paulmelnikow Apr 22, 2022
78b886c
Merge branch 'master' into express
paulmelnikow Apr 22, 2022
18c76e3
Clean up acceptor test
paulmelnikow Apr 22, 2022
9d6b3e0
Clean lint
paulmelnikow Apr 22, 2022
25ab480
Clean up makeBadge tests
paulmelnikow Apr 22, 2022
416ef39
Don't alias `this`
paulmelnikow Apr 22, 2022
1761aab
Wee bit of cleanup
paulmelnikow Apr 22, 2022
fc13e8e
Merge branch 'master' into express
paulmelnikow Apr 22, 2022
ca6ae88
Tweak style
paulmelnikow Apr 22, 2022
fc68b88
Merge branch 'master' into express
paulmelnikow Apr 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions badge-maker/lib/make-badge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict'

const { normalizeColor, toSvgColor } = require('./color')
const { toSvgColor } = require('./color')
const badgeRenderers = require('./badge-renderers')
const { stripXmlWhitespace } = require('./xml')

Expand All @@ -9,7 +9,6 @@ note: makeBadge() is fairly thinly wrapped so if we are making changes here
it is likely this will impact on the package's public interface in index.js
*/
module.exports = function makeBadge({
format,
style = 'flat',
label,
message,
Expand All @@ -24,22 +23,6 @@ module.exports = function makeBadge({
label = `${label}`.trim()
message = `${message}`.trim()

// This ought to be the responsibility of the server, not `makeBadge`.
if (format === 'json') {
Copy link
Member Author

Choose a reason for hiding this comment

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

I've argued before that this functionality doesn't belong in the badge library as the particular JSON schema we generate here (with name and value) is a legacy quirk of shields.io, and not something we particularly want users to replace elsewhere. For the moment I went ahead and moved this functionality to the server. It's now handled by makeJsonBadge() which is invoked from a conditional in the request handler in base.js.

If we keep this we'll probably want to add normalizeColor to the package's public interface, or alternatively a normalizeBadgeData() function, and then we could start dogfooding the public makeBadge().

Copy link
Member

Choose a reason for hiding this comment

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

I haven't read over the code for this bit at all yet. I agree with the objective. Has anything significant has changed since #4980 in terms of the implementation, specifically #4980 (comment) ? I will have a look through it. If a slightly different configuration of not eating our own dogfood is necessary to get off scoutcamp, this shouldn't be a blocker but until we do #4947 and #4949 I suspect all we can really do is move from one undesirable implementation to another. I do accept #4947 and #4949 have been open for 2 years and none of us are actively working on them.
As a note, we may want to consider documenting it as a non-BC change for badge-maker, even though format='json' is undocumented. The next badge-maker will be a major release anyway due to dropping node 10.

return JSON.stringify({
label,
message,
logoWidth,
// Only call normalizeColor for the JSON case: this is handled
// internally by toSvgColor in the SVG case.
color: normalizeColor(color),
labelColor: normalizeColor(labelColor),
link: links,
name: label,
value: message,
})
}

const render = badgeRenderers[style]
if (!render) {
throw new Error(`Unknown badge style: '${style}'`)
Expand Down
Loading