-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add Prettier and recommended ESLint rules. #351
Conversation
* main: Add CSS toGamut algorithm (color-js#344) Accept "Lightness" for lab space first channel name (color-js#348) Fix type def for MixOptions (color-js#347) Add CJS file to /fn entry and fix legacy builds (color-js#349)
Hi @jgerigmeyer just a quick note that I'm not ignoring this, just want to prepare a thoughtful response that I have not had time to finish. Thank you for your time and patience! |
No rush -- I recognize there are often a lot of opinions around this! 😄 I'm not tied to this PR, just wanted to propose it. |
Hi again @jgerigmeyer, I started replying to this PR and it turned into a blog post. Here's a draft: https://lea.verou.me/blog/2023/formatters/
Curious why would you prefer that over just setting your tab width to 2 so others can still see the code with their preferred tab width? Happy thanksgiving! |
Yeah, I think you're right on this 👍
@LeaVerou Thanks -- this is a more thought-out response than I anticipated. 😅 I don't have strong feelings, and think your points are valid -- especially for a library like Color.js. Some minor thoughts:
I think my view is still that the benefits of Prettier often outweigh the cons, and I lean toward "use Prettier and override it in a few select places as-needed" for most codebases -- but I don't have particular counter-arguments to the issues you raise, and it seems like it's not the right fit for Color.js. I'll close this PR but leave it intact for historical purposes since it's linked from your blog post, but would you entertain a new PR that expands on the current usage of ESLint and enforces some additional recommended rules? |
Yes! One more converted, we'll get there 😅
Yup, but the argument for Prettier was that contributors don't need to expend mental energy trying to determine stylistic expectations. Unless they have it locally installed, they absolutely do. And yes, that's also true with ESLint.
Yup, we should absolutely do that!
That's where I disagree. I think scannability/readaiblity is more important than consistency, and we can enforce the latter manually in PRs until that style guide is written (and ESLint can help a lot too, so thank you for that other PR!)
Oops, sorry, will fix!
I think that's the main issue: I’m opposed to adding overrides in the codebase, and with Prettier there is no way around it. At least with ESLint you can remove or relax the rule in question.
Absolutely, thank you so much!! We'll probably need to iterate a bit, but I totally see this one getting merged eventually. |
I know these are subjective and I'm very open to changes, but I'm proposing a clearer and more opinionated (and automated) set of style/formatting rules. I see two main advantages:
(I'd also personally prefer a switch from tabs to 2-spaces, but I'm not looking to start any fights 😅)
@LeaVerou @svgeesus Thoughts?