Refactor badge color functions #2742

paulmelnikow · 2019-01-12T04:59:48Z

Replace the idea of color schemes with the idea of named colors (since none of our colorschemes have used colorA)
Pass through the normalized color to _shields_test to harmonize with BaseService and simplify testing
- Update service tests
Move responsibility for color generation into the npm package
Remove several color helper functions and their tests
Update gh-badge public API to accept color and labelColor

This is a precursor to refactoring some of the logo code for #2473.

- Replace the idea of color schemes with the idea of named colors (since none of our colorschemes have used `colorA`) - Pass through the normalized color to `_shields_test` to harmonize with BaseService and simplify testing - Update service tests - Move responsibility for color generation into the npm package - Remove several color helper functions and their tests - Update gh-badge public API to accept `color` and `labelColor`

shields-ci · 2019-01-12T05:00:41Z

	Warnings
⚠️	This PR modified `package.json`, but not `package-lock.json` - Perhaps you need to run `npm install`?
⚠️	This PR modified service code for `cocoapods` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `microbadger` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `twitter` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	📚 Remember to ensure any changes to `serverSecrets` in `services/wheelmap/wheelmap.tester.js` are reflected in the server secrets documentation

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

Generated by 🚫 dangerJS against 9cffbe5

paulmelnikow · 2019-01-12T22:25:29Z

This pull request introduces 1 alert when merging 628618f into fc4ed93 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

Comment posted by LGTM.com

paulmelnikow · 2019-01-12T22:25:51Z

Confident at this point that the failing service tests are unrelated to these changes.

chris48s · 2019-01-13T13:57:08Z

I've had a go at reviewing this. I'll leave a few comments but it probably needs another pass.

One of the issue I'm hitting trying to test is that editing stuff in the modal popups in the front end is throwing:

TypeError: Cannot read property 'toLowerCase' of null
at isHexColor (shields/gh-badges/lib/color.js:22:31)

which is making it difficult to test overrides, logos etc

gh-badges/lib/badge-cli.js

chris48s · 2019-01-13T13:58:10Z

services/base.js

+      color = coalesce(serviceColor, defaultColor, 'lightgrey')
+    } else {
+      color = coalesce(overrideColor, serviceColor, defaultColor, 'lightgrey')
+    }


This section will want updating to account for #2740

services/dub/dub.tester.js

# Conflicts: # services/dub/dub.tester.js

paulmelnikow · 2019-01-13T16:26:10Z

One of the issue I'm hitting trying to test is that editing stuff in the modal popups in the front end is throwing:
TypeError: Cannot read property 'toLowerCase' of null
at isHexColor (shields/gh-badges/lib/color.js:22:31)
which is making it difficult to test overrides, logos etc

Ah, thanks. Given TypeScript, the preferred null value these days is undefined. I made the color function the slightest bit less lenient in that it no longer accepts null, though the frontend seems to be one more place where we were passing it that.

paulmelnikow · 2019-01-13T16:29:51Z

Wait, sorry, I can't seem to reproduce the problem you're seeing. What are the steps to reproduce?

I did just merge master into this branch so it's possible but unlikely that took care of it.

gh-badges/lib/color.js

calebcartwright · 2019-01-14T23:11:45Z

Should we also run these changes against all service tests to validate there's no unexpected color change behavior?

paulmelnikow · 2019-01-14T23:37:57Z

Should we also run these changes against all service tests to validate there's no unexpected color change behavior?

Yea, I did the full service test run in a number of the earlier builds, and cleaned up all the failures that seemed to be related. It's possible one or two have crept in since then, though at this point I think we can catch them more easily just after merging.

paulmelnikow · 2019-01-14T23:40:21Z

Gotcha. Yes! Thanks a bunch. Looks like a backend issue. I'll see if I can't track that down.

@chris48s This issue is fixed, by the way.

calebcartwright · 2019-01-14T23:44:40Z

Yea, I did the full service test run in a number of the earlier builds

Ah sorry, I missed that

calebcartwright · 2019-01-14T23:56:20Z

Tangential question, but have we ever evaluated the feasibility of having Coveralls chime in on PRs with info about coverage changes? I imagine that could get tricky given the different test runs we have (and I know those coverage bots can get a little spammy on some PRs), but I think that could be valuable in some cases, especially for the core code.

As was mentioned in a different thread the other day, there's ~40k lines of code in this project, most of which I'm still personally unfamiliar with. I know for me it would be really helpful during some PR reviews to be able to see that all the tests for that code are passing and coverage is 100% (or at least hasn't dropped), especially when dealing with sections of the code base I'm unfamiliar with

paulmelnikow · 2019-01-15T00:05:55Z

There was a little discussion in #1584 and I wrote a bit the other day in #2756 (comment).

I think ideally we have two suites:

Core
- Only runs the core tests
- Is expected to climb toward 100%
- Is reported in PRs
Services
- Runs the service tests specified in PRs
- Gets reported in PRs
- Ideally, only includes code belonging to the services being run. In some cases it may be difficult to detect which service code should be included. If we run [pypidownloads] then which files in pypi should be covered? I suppose if we run a subset of a service's tests we could err on the side of under-reporting.

1 seems easier to build. We could start there. Ideally if we can find a way to hack this with Coveralls, I think we should do that… it'd involve somehow keeping the two coverage runs separate. It might be possible if we switch daily-tests to think it's reporting on a branch, though I'm not totally sure if that would work.

calebcartwright · 2019-01-15T00:10:56Z

I agree with targetting/emphasizing the 1. Core option

I suppose we could leverage multiple .nycrc files, and perhaps another option would be to use Coveralls for one run and maybe a different service like Codecov for another? I'd venture to say others have encountered a similar scenario before (for example monorepos) and wanted coverage on different parts so I'd be curious to see what others have done.

calebcartwright · 2019-01-15T00:12:51Z

Also as I read through #1584 I realize I am just repeating things you've all already thought of 😆

calebcartwright · 2019-01-15T00:18:58Z

While searching for Coveralls docs I found this from codecov: https://docs.codecov.io/docs/flags. I'll see what I can find on Coveralls and will open another issue if need be to avoid taking this PR any more off topic!

paulmelnikow · 2019-01-15T03:47:00Z

Ooh, possibly useful: https://stackoverflow.com/a/49907438/893113

chris48s

Sorry - hadn't clocked this was fixed. Done a bit more testing. Should be good to go

paulmelnikow · 2019-01-15T20:47:15Z

Fabulous! Thanks for reviewing. I know this was kind of a bear. 🐻

Merge conflict sorted.

shields-deployment · 2019-01-15T21:43:36Z

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:

Numeric colors weren't properly being handled by `makeBadge` after #2742. Since this function really does not need to be accepting colors as strings, rather than make the function more lenient to work with Scoutcamp, I coerced the types of the colors on the way in. Two tests cover the functionality in the modern service. I don't feel strongly that the legacy version needs coverage at this point, though I've added one for the moment on the github languages badge where this manifested. Fix #2778

paulmelnikow added service-badge Accepted and actionable changes, features, and bugs core Server, BaseService, GitHub auth npm-package Badge generation and badge templates labels Jan 12, 2019

paulmelnikow added the blocker PRs and epics which block other work label Jan 12, 2019

paulmelnikow changed the title ~~Refactor badge color functions~~ Refactor badge color functions [*] Jan 12, 2019

Fix failing tests

ae31316

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 12, 2019 17:03 Inactive

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 12, 2019 17:14 Inactive

More fixes

24c1178

paulmelnikow force-pushed the refactor-badge-color branch from 95876a0 to 24c1178 Compare January 12, 2019 17:22

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 12, 2019 17:22 Inactive

Fix GithubIssueDetail

628618f

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 12, 2019 22:19 Inactive

Clean lint

715b38a

paulmelnikow changed the title ~~Refactor badge color functions [*]~~ Refactor badge color functions Jan 12, 2019

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 12, 2019 22:26 Inactive

chris48s reviewed Jan 13, 2019

View reviewed changes

paulmelnikow added 3 commits January 13, 2019 11:20

Update CLI doc

b8b2a8d

Merge branch 'master' into refactor-badge-color

a392fb9

# Conflicts: # services/dub/dub.tester.js

Update dub

392b6cb

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 13, 2019 16:24 Inactive

Run prettier

c500b83

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 13, 2019 16:30 Inactive

calebcartwright reviewed Jan 13, 2019

View reviewed changes

gh-badges/lib/color.js Show resolved Hide resolved

Add comment about hex colors with leading hash

a8de7d2

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 13, 2019 21:05 Inactive

Merge branch 'master' into refactor-badge-color

223dfa3

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 14, 2019 23:40 Inactive

paulmelnikow mentioned this pull request Jan 14, 2019

Automatically show light or dark logos depending on left color #2431

Closed

Merge branch 'master' into refactor-badge-color

50de142

paulmelnikow temporarily deployed to shields-staging-pr-2742 January 15, 2019 11:26 Inactive

chris48s previously approved these changes Jan 15, 2019

View reviewed changes

Merge branch 'master' into refactor-badge-color

9cffbe5

paulmelnikow dismissed chris48s’s stale review via 9cffbe5 January 15, 2019 20:46

paulmelnikow deployed to shields-staging-pr-2742 January 15, 2019 20:46 View deployment

chris48s approved these changes Jan 15, 2019

View reviewed changes

paulmelnikow merged commit 4597d77 into master Jan 15, 2019

paulmelnikow deleted the refactor-badge-color branch January 15, 2019 21:43

This was referenced Jan 16, 2019

Error: Vendor hook error:,TypeError: s.toLowerCase is not a function #2778

Closed

Fix numeric colorB [static githublanguages] #2780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor badge color functions #2742

Refactor badge color functions #2742

paulmelnikow commented Jan 12, 2019 •

edited

Loading

shields-ci commented Jan 12, 2019 •

edited

Loading

paulmelnikow commented Jan 12, 2019

paulmelnikow commented Jan 12, 2019

chris48s commented Jan 13, 2019

chris48s Jan 13, 2019

paulmelnikow commented Jan 13, 2019

paulmelnikow commented Jan 13, 2019

calebcartwright commented Jan 14, 2019

paulmelnikow commented Jan 14, 2019

paulmelnikow commented Jan 14, 2019

calebcartwright commented Jan 14, 2019

calebcartwright commented Jan 14, 2019 •

edited

Loading

paulmelnikow commented Jan 15, 2019

calebcartwright commented Jan 15, 2019

calebcartwright commented Jan 15, 2019

calebcartwright commented Jan 15, 2019

paulmelnikow commented Jan 15, 2019

chris48s left a comment

paulmelnikow commented Jan 15, 2019

shields-deployment bot commented Jan 15, 2019

Refactor badge color functions #2742

Refactor badge color functions #2742

Conversation

paulmelnikow commented Jan 12, 2019 • edited Loading

shields-ci commented Jan 12, 2019 • edited Loading

paulmelnikow commented Jan 12, 2019

paulmelnikow commented Jan 12, 2019

chris48s commented Jan 13, 2019

chris48s Jan 13, 2019

Choose a reason for hiding this comment

paulmelnikow commented Jan 13, 2019

paulmelnikow commented Jan 13, 2019

calebcartwright commented Jan 14, 2019

paulmelnikow commented Jan 14, 2019

paulmelnikow commented Jan 14, 2019

calebcartwright commented Jan 14, 2019

calebcartwright commented Jan 14, 2019 • edited Loading

paulmelnikow commented Jan 15, 2019

calebcartwright commented Jan 15, 2019

calebcartwright commented Jan 15, 2019

calebcartwright commented Jan 15, 2019

paulmelnikow commented Jan 15, 2019

chris48s left a comment

Choose a reason for hiding this comment

paulmelnikow commented Jan 15, 2019

shields-deployment bot commented Jan 15, 2019

paulmelnikow commented Jan 12, 2019 •

edited

Loading

shields-ci commented Jan 12, 2019 •

edited

Loading

calebcartwright commented Jan 14, 2019 •

edited

Loading