Add option for IDs to have a suffix

[randomairborne]

This PR attempts to fix the issue i ran into #10575, discussion #10321, and original issue #10336. It adds an optional prefix to ID values, so that multiple invocations can use different ID values. This prevents needing long random identifiers and increasing size in the general case, and gives users that need it more flexibility.

[github-actions]

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

Generated by 🚫 dangerJS against 1d802a4

[randomairborne]

maybe this needs to be documented more? I'm not quite sure where that is, though. It's not really a complex feature 😅

[chris48s]

Thanks. I don't mind this as an approach. It does allow you to generate badges with unique ids. This does push the work of doing that onto the user, but one thing this does give you is predictable ids. i.e: if we use UUIDs then the same rect will have a different id every time you generate the badge, whereas with this you can give it the same id every time. That might be useful, I guess.

I think I would make this a suffix rather than a prefix though. I would assume one of the ways people might use this feature to generate unique ids would be to use a loop/counter variable as the "unique" part of the ID, but with a prefix this would generate ids that start with a number, which is invalid. If we make it a suffix then we control the first character of the ID and we can ensure the IDs are valid rather than pushing that on to the user and giving them a foot-gun here.

In terms of docs, we will need to cover this in the badge-maker readme (mainly what problem does this feature solve), but lets nail the feature down first.

I would also want this feature to be covered by tests before we review it.

[randomairborne]

Yeah, my thought was that "hmm, a uuid is really long to use by default". I'll add some tests (Where's the best place to do that?) and i'll document it in the readme.

[chris48s]

Thanks.

In terms of the tests, the most important place to cover this is the snapshot tests in badge-maker/lib/make-badge.spec.js

These are quite easy to add. If you add some new test cases to this file, the first time you run npm run test:package a snapshot of the SVG output will automatically be added to __snapshots__/make-badge.spec.js. Just check the snapshots over and then once the SVG output is stored, that gives us regression tests for future. On future test runs, we'll compare the output of the function to the stored shapshot for that named test case.

It would also be useful to add a case for the param validation to the "should throw a ValidationError with invalid inputs" tests in badge-maker/lib/index.spec.js

[randomairborne]

Does this count as a breaking change for the purposes of version bumps? is there a release cadence for this? I'd like to end up using it, but I'm not sure when I would need to set a reminder.

[chris48s]

I checked this out and had a look. Rather than going back and forth on this I've pushed a few more commits making a few final tweaks.

In terms of the release, this is not a breaking change. We are just adding a feature. We're not removing anything or making any behaviour changes to existing functionality. The code makeBadge({ label: 'build', message: 'passed', color: 'brightgreen' }) will output the exact same SVG before and after this change so the next release can just be 4.1.0. Tbh we don't change the SVG rendering logic very much so there is not a defined release schedule for badge-maker. I'll just merge this and push a 4.1.0 release to NPM with this change in it today.

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher

🚮 Removed packages: npm/@actions/core@1.11.1, npm/@actions/github@6.0.0

View full report↗︎

[randomairborne]

Thank you very much.