-
Notifications
You must be signed in to change notification settings - Fork 8
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
Simplify package structure and decouple from Airbnb #20
Conversation
I think this is ready for a fist review. |
Added commits to enable |
package.json
Outdated
"dependencies": { | ||
"coffeelint-config-groupon": "file:./packages/coffeelint-config-groupon", | ||
"coffeelint-forbidden-keywords": "~0.1.1", | ||
"coffeelint-no-mocha-only": "^1.0.0", | ||
"coffeelint-use-strict": "^1.0.0", | ||
"coffeescope2": "~0.4.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be peerDeps instead of direct deps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, good point. Yeah, I think so. Blindly left these here based on the previous top-level state which is clearly not correct.
package.json
Outdated
"name": "groupon-styleguide", | ||
"version": "3.2.0", | ||
"private": true, | ||
"name": "eslint-config-groupon", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean for coffeelint it'll be "extends": "eslint-config-groupon/coffee"
(similarly for stylus/css?) should we choose a new module name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eslint
is the only one that cares about the naming. In theory we could just go for groupon-code-style
or similar. IIRC eslint
also accepts full package names..?
Shouldn't this be semver-major? |
Re major: Yep! Let me reword the first commit to include the marker. |
README.md
Outdated
@@ -39,82 +26,25 @@ This ensures that: | |||
|
|||
Fortunately there already is a great and well-documented style guide for JavaScript over at [airbnb/javascript](https://github.com/airbnb/javascript). | |||
The short answer is: We stick to that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even true or worth saying anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should reword this. Maybe turn it into an acknowledgement of where we started.
BREAKING CHANGE: This switches our linting to an entirely new set of rules. For the most part we're trying to stay consistent with the old format to reduce churn but it explicitly is a departure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐑 it
prettier
for even more automated fixingeslint@4
Still pending:
Port CSS formatting to prettiernlm
(again) and make it publishable.Rules that didn't make the cut (selection):
x++
and mixed operators. No--fix
yet.--fix
solution.I tried to be conservative about adding back rules in general. I think we cover all the actual bug prevention now and
prettier
should cover formatting.