-
Notifications
You must be signed in to change notification settings - Fork 85
Add prettier + eslint #89
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
{ | ||
"extends": "@segment/eslint-config/browser/legacy" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we still need to keep the old config around? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, that makes sense - we definitely need that 🙏 |
||
"extends": ["@segment/eslint-config/browser/legacy", "prettier"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"singleQuote": true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? What does it do? I see our internal app codebase using single quotes but not have this prettier rule. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah nvm, they pass it as an option in the binary instead: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we remove it |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ | |
# Binaries | ||
## | ||
|
||
ESLINT := node_modules/.bin/eslint | ||
KARMA := node_modules/.bin/karma | ||
|
||
## | ||
|
@@ -58,12 +57,13 @@ distclean: clean | |
|
||
# Lint JavaScript source files. | ||
lint: install | ||
@$(ESLINT) $(ALL_FILES) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're not using ESLint anymore in the makefile, we could remove it from line 5 as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Today's free tip: with newer versions of yarn you can directly invoke |
||
yarn lint | ||
|
||
.PHONY: lint | ||
|
||
# Attempt to fix linting errors. | ||
fmt: install | ||
@$(ESLINT) --fix $(ALL_FILES) | ||
yarn format | ||
.PHONY: fmt | ||
|
||
# Run browser unit tests in a browser. | ||
|
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.
Had to upgrade to 8 for
lint-staged
, see CI Job#14There 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.
Would this have any effect on the final functionality?
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.
The final artifact is built by the analytics.js repo so it should only affect tests