Add more ESLint recommended rules, and enforce them in CI. #373

jgerigmeyer · 2023-11-29T19:00:40Z

A replacement of #351 and #367.

Right now CI will fail if there are any lint errors or warnings.

Very open to feedback and/or rules changes!

This reverts commit 82d2ce9.

* main: More consistent formatting across the codebase. (color-js#372) More accurate ICtCp matrices (color-js#365) Fix Jzazbz tests (color-js#366)

netlify · 2023-11-29T19:00:51Z

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	`afdba27`
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/6581eee6adb2bb0008661ebe
😎 Deploy Preview	https://deploy-preview-373--colorjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jgerigmeyer · 2023-11-29T19:33:14Z

@LeaVerou Sorry for so many PRs! I forgot to rebase #367 before merging #372, so it got closed automatically. 🤦

LeaVerou

Thanks so much!

Main issues are:

I'd rather avoid black boxes of rules like eslint:recommended, and list the rules we want explicitly
I know it's a lot of work, but it would be very useful if we could add comments above rules with obscure names (even for those with non-obscure names I find myself wondering about the specific, e.g. keyword-spacing is obvously about keyword spacing, but what kind of spacing does it enforce?). Is there something that could automate this perhaps?

Presumably we can still merge something even if CI fails, right? Something worth exploring might be to only fail CI for the more egregious violations and not all style rules.

LeaVerou · 2023-11-29T19:35:39Z

.eslintrc.cjs

+	},
+	"env": {
+		"browser": true,
+		"es6": true,


Rather than specifying a specific version, is there a way to say "the newer version supported"?

The purpose of this is to tell ESLint what predefined global variables are assumed to be available. My understanding is that Color.js supports down to Node v12 with legacy builds, and Node v16+ with the base build (and equivalent browsers). We could safely update this e.g. es2020 based on that -- but it also doesn't hurt for this to be a lower version unless/until we start using cutting-edge ES functionality.

I'll update to es2020 for now. (I don't think there's a way to make this a dynamic value, no.)

https://eslint.org/docs/latest/use/configure/language-options#specifying-environments

jgerigmeyer

I'd rather avoid black boxes of rules like eslint:recommended, and list the rules we want explicitly

Adjusted in da191b1

I know it's a lot of work, but it would be very useful if we could add comments above rules with obscure names (even for those with non-obscure names I find myself wondering about the specific, e.g. keyword-spacing is obvously about keyword spacing, but what kind of spacing does it enforce?). Is there something that could automate this perhaps?

Added in 93bfb52

Presumably we can still merge something even if CI fails, right? Something worth exploring might be to only fail CI for the more egregious violations and not all style rules.

Yes, we can still merge if CI fails. If we decide some rules are worth CI failures and others aren't, the simplest solution would be to mark the high priority ones as error and tell CI to only fail on errors.

jgerigmeyer · 2023-11-29T21:45:03Z

apps/blackwhite/style.postcss

 		order: var(--c);
 	}

-	@nest [data-order="h"] & {
+	[data-order="h"] & {


These were causing deprecation warnings.

jgerigmeyer · 2023-11-29T21:45:45Z

babel.config.json

@@ -5,7 +5,7 @@
 			{
 				"targets": "defaults, node >= 12",
 				"useBuiltIns": "usage",
-				"corejs": "3.25"
+				"corejs": "3.30"


Updated to match the current installed version, as recommended by Babel.

jgerigmeyer · 2023-11-29T21:46:25Z

package.json

+		"lint": "run-s \"eslint -- --fix\" dtslint",
+		"lint:ci": "run-s \"eslint -- --max-warnings 0\" dtslint",


Locally npm run lint will pass, but display warnings. In CI, lint warnings will cause CI to fail.

tests/README.html

LeaVerou

Sorry for the delay, and thanks again so much for this work. Amazing stuff. I think once we're done with this, I'll use this eslint file across my other repos too!

I've left a few comments. I didn't check the TS rules, as long as you and @MysteryBlokHed have consensus on these, feel free to use whatever.

As a general point, I'd love it if we could put the explanatory comments above the rule they describe, so we could have space to also include a link (e.g. https://eslint.org/docs/latest/rules/no-unsafe-optional-chaining or https://eslint.style/rules/default/brace-style ). Even with the comments, I still had to look up a lot of these rules to understand what they did, and having clickable links would have been very convenient. But also, I'm wary of asking you to do more work for this, so take this as a non-blocking suggestion. :)

.eslintrc.cjs

LeaVerou · 2023-12-16T20:35:17Z

Hi @jgerigmeyer, I’m quite eager to merge this, how could I help?

jgerigmeyer · 2023-12-16T20:37:23Z

@LeaVerou Sorry for the delay! I've been swamped with travel and work. I'll prioritize this Monday morning 👍🏻

* main: Add CAM16 (JMh) (color-js#379) [spaces/prophoto] Use 64 bit-accurate conversion matrices formatting [spaces/hsl] Better handling of negative saturation on very oog colors Point to the actual, working tests not the old ones CSS4 toGamut fixes (color-js#352)

jgerigmeyer

As a general point, I'd love it if we could put the explanatory comments above the rule they describe, so we could have space to also include a link.

Added in cae6066

But also, I'm wary of asking you to do more work for this, so take this as a non-blocking suggestion. :)

Not a problem! Sorry for the delay.

jgerigmeyer · 2023-12-19T03:22:26Z

src/spaces/cam16.js

@@ -195,7 +195,7 @@ export function fromCam16(cam16, env) {
 		hRad = constrain(cam16.h) * deg2rad;
 	}
 	else {
-		h_rad = invHueQuadrature(H) * deg2rad;
+		hRad = invHueQuadrature(cam16.H) * deg2rad;


@facelessuser I am not confident in these changes, but the new lint rules highlighted that h_rad and H were both undefined here. Can you clarify the intention?

Yeah, h_rad was a typo on my part, it should be hRad as you've done. We aren't currently generating the reverse transform with hue quadrature, so this wasn't caught.

H is not likely to be undefined as we check to make sure h or H is defined on line 183. So one of them must be defined at this point, but it may confuse the checker.

Thanks! So it sounds like this is good to go.

H is not likely to be undefined as we check to make sure h or H is defined on line 183. So one of them must be defined at this point, but it may confuse the checker.

H was undefined (likely a typo) -- this changes it to cam16.H, which is defined.

Oh, H was undefined! 🤦🏻 Sorry, this all looks correct.

jgerigmeyer · 2023-12-19T03:24:24Z

@LeaVerou Updated and ready for another round of review!

LeaVerou · 2023-12-19T17:12:20Z

src/util.js

@@ -241,7 +241,7 @@ export function zdiv(n, d) {
 * @param {number} hi - used to specify a the high end of a subset of the list
 * @returns number
 */
-export function bisectLeft(arr, value, lo=0, hi=arr.length) {
+export function bisectLeft (arr, value, lo=0, hi=arr.length) {


Non-blocking but we may want to add a rule that the = here needs spaces around it.

@LeaVerou I added a few more spacing-related rules in 0e14ea1 -- let me know if you want me to remove any of them.

tests/README.html

LeaVerou

LGTM! Just a rebase needed to remove this README.html

Please make sure to squash-merge since there are a lot of commits.

Thanks again so much for working on this!!

* main: Do not generate a tests/README.html! Add functions to convert old tests to new tests

LeaVerou · 2023-12-19T18:06:54Z

There was nothing specific about that =? We can add more later, I was hoping to merge this today but don't have cycles for the kind of more thorough review needed for adding extra rules. :/

jgerigmeyer · 2023-12-19T19:29:06Z

There was nothing specific about that =? We can add more later, I was hoping to merge this today but don't have cycles for the kind of more thorough review needed for adding extra rules. :/

@LeaVerou The three rules I added are:

space-before-blocks: Enforce at least one space before blocks
space-infix-ops: Require spaces around infix operators (e.g. +, =, ?, :)
space-unary-ops: Enforce a space after unary word operators (new, delete, typeof, void, yield)

I think they're pretty straightforward and I've looked carefully through the resulting code changes and don't see any issues, but if you prefer I can remove them.

LeaVerou · 2023-12-19T23:56:28Z

Ok, let's merge this! 🚀

jgerigmeyer added 7 commits November 28, 2023 12:41

Add more ESLint recommended rules, and enforce them in CI.

9073d1a

Additional linting

c1f0fad

Remove eslint comments and relax rules

9fa48ac

Enable typescript-eslint stylistic rules

0fa47fb

Remove eslint changes

82d2ce9

Revert "Remove eslint changes"

aac6e62

This reverts commit 82d2ce9.

Merge branch 'main' into formatting-v2

4002a47

* main: More consistent formatting across the codebase. (color-js#372) More accurate ICtCp matrices (color-js#365) Fix Jzazbz tests (color-js#366)

jgerigmeyer requested a review from LeaVerou November 29, 2023 19:00

jgerigmeyer mentioned this pull request Nov 29, 2023

Add more ESLint recommended rules, and enforce them in CI. #367

Closed

LeaVerou requested changes Nov 29, 2023

View reviewed changes

jgerigmeyer added 2 commits November 29, 2023 15:48

Remove eslint extensions

da191b1

Add descriptions to linting rules

93bfb52

jgerigmeyer commented Nov 29, 2023

View reviewed changes

jgerigmeyer requested a review from LeaVerou November 29, 2023 21:50

LeaVerou requested changes Dec 6, 2023

View reviewed changes

jgerigmeyer added 3 commits December 18, 2023 22:15

Update lint config

cae6066

lint

349026e

jgerigmeyer commented Dec 19, 2023

View reviewed changes

jgerigmeyer requested review from LeaVerou and facelessuser December 19, 2023 03:23

LeaVerou reviewed Dec 19, 2023

View reviewed changes

tests/README.html Outdated Show resolved Hide resolved

LeaVerou approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'main' into formatting-v2

1ada4d6

* main: Do not generate a tests/README.html! Add functions to convert old tests to new tests

jgerigmeyer added 2 commits December 19, 2023 12:42

Add more spacing lint rules

0e14ea1

Remove tests/README.html

6d15c46

facelessuser approved these changes Dec 19, 2023

View reviewed changes

clarify comments

afdba27

jgerigmeyer merged commit c04f61c into color-js:main Dec 20, 2023
4 checks passed

jgerigmeyer deleted the formatting-v2 branch December 20, 2023 01:00

apiiro-snyk mentioned this pull request Jul 3, 2024

[Snyk] Upgrade colorjs.io from 0.4.5 to 0.5.0 apiiro-snyk/halo#3

Open

Carolinewly mentioned this pull request Aug 4, 2024

[Snyk] Upgrade colorjs.io from 0.4.5 to 0.5.2 Carolinewly/halo#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more ESLint recommended rules, and enforce them in CI. #373

Add more ESLint recommended rules, and enforce them in CI. #373

jgerigmeyer commented Nov 29, 2023

netlify bot commented Nov 29, 2023 •

edited

Loading

jgerigmeyer commented Nov 29, 2023

LeaVerou left a comment

LeaVerou Nov 29, 2023

jgerigmeyer Nov 29, 2023 •

edited

Loading

jgerigmeyer left a comment

jgerigmeyer Nov 29, 2023

jgerigmeyer Nov 29, 2023

jgerigmeyer Nov 29, 2023

LeaVerou left a comment

LeaVerou commented Dec 16, 2023

jgerigmeyer commented Dec 16, 2023

jgerigmeyer left a comment

jgerigmeyer Dec 19, 2023

facelessuser Dec 19, 2023 •

edited

Loading

jgerigmeyer Dec 19, 2023

facelessuser Dec 19, 2023

jgerigmeyer commented Dec 19, 2023

LeaVerou Dec 19, 2023

jgerigmeyer Dec 19, 2023

LeaVerou left a comment

LeaVerou commented Dec 19, 2023

jgerigmeyer commented Dec 19, 2023

LeaVerou commented Dec 19, 2023

		"lint": "run-s \"eslint -- --fix\" dtslint",
		"lint:ci": "run-s \"eslint -- --max-warnings 0\" dtslint",

Add more ESLint recommended rules, and enforce them in CI. #373

Add more ESLint recommended rules, and enforce them in CI. #373

Conversation

jgerigmeyer commented Nov 29, 2023

netlify bot commented Nov 29, 2023 • edited Loading

✅ Deploy Preview for colorjs ready!

jgerigmeyer commented Nov 29, 2023

LeaVerou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgerigmeyer Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

jgerigmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou left a comment

Choose a reason for hiding this comment

LeaVerou commented Dec 16, 2023

jgerigmeyer commented Dec 16, 2023

jgerigmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facelessuser Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgerigmeyer commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou left a comment

Choose a reason for hiding this comment

LeaVerou commented Dec 19, 2023

jgerigmeyer commented Dec 19, 2023

LeaVerou commented Dec 19, 2023

netlify bot commented Nov 29, 2023 •

edited

Loading

jgerigmeyer Nov 29, 2023 •

edited

Loading

facelessuser Dec 19, 2023 •

edited

Loading