Skip to content
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 linter config for schemas #415

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

o-rumiantsev
Copy link
Member

No description provided.

@o-rumiantsev o-rumiantsev requested a review from nechaido April 18, 2019 11:44
@o-rumiantsev o-rumiantsev self-assigned this Apr 18, 2019
.eslintignore Outdated Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
.eslintignore Outdated
@@ -0,0 +1,8 @@
data/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point it would be easier to specify only the files that should be linted, not the files that must be ignored:

Suggested change
data/
*
!*.js
!*.action
!*.category
!*.domains
!*.form

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eslint will lint only .js files, if there is no extensions specified using --ext. So lint script will look like this:

eslint --ext .js,.action,.category,.domains,.form . && prettier -c \"**/*\"

package.json Outdated
@@ -34,8 +34,8 @@
],
"scripts": {
"test": "npm run lint && metatests test/",
"lint": "eslint . --ignore-path .gitignore && prettier -c \"**/*.js\" \"**/*.json\" \"**/*.md\" \".*rc\" \"**/*.yml\"",
"fmt": "prettier --write \"**/*.js\" \"**/*.json\" \"**/*.md\" \".*rc\" \"**/*.yml\"",
"lint": "eslint \"**/*\" && prettier -c \"*\"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"lint": "eslint \"**/*\" && prettier -c \"*\"",
"lint": "eslint \"**/*\" && prettier -c \"**/*\"",

package.json Outdated
"lint": "eslint . --ignore-path .gitignore && prettier -c \"**/*.js\" \"**/*.json\" \"**/*.md\" \".*rc\" \"**/*.yml\"",
"fmt": "prettier --write \"**/*.js\" \"**/*.json\" \"**/*.md\" \".*rc\" \"**/*.yml\"",
"lint": "eslint \"**/*\" && prettier -c \"*\"",
"fmt": "eslint --fix \"**/*\" && prettier --write \"*\"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"fmt": "eslint --fix \"**/*\" && prettier --write \"*\"",
"fmt": "eslint --fix \"**/*\" && prettier --write \"**/*\"",

.prettierignore Outdated
@@ -1,2 +1,7 @@
data
dist
data/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for the .eslintignore.

Copy link
Member Author

@o-rumiantsev o-rumiantsev May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have bumped into a strange behavior of .prettierignore.
I have tried this, and it lints only gs.js file and does not go deeper.

*
!*.js

Then I have tried this, and nothing have changed.

*
!**/*.js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same with .eslintignore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o-rumiantsev, right, I forgot to add the !*/ rule after the *.

.prettierignore Outdated Show resolved Hide resolved
@o-rumiantsev o-rumiantsev force-pushed the add-linter-config-for-schemas branch 2 times, most recently from dd48755 to 51e7b98 Compare June 13, 2019 09:33
@nechaido nechaido requested a review from belochub July 1, 2019 07:05
@o-rumiantsev o-rumiantsev force-pushed the add-linter-config-for-schemas branch from 51e7b98 to 081636e Compare July 3, 2019 06:17
@o-rumiantsev
Copy link
Member Author

o-rumiantsev commented Jul 3, 2019

@belochub

.prettierignore Outdated Show resolved Hide resolved
.prettierignore Outdated
!*.action
!*.category
!*.domains
!*.form
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
!*.form
!*.form
!*.application

?

@@ -1,3 +1,4 @@
/* eslint-disable no-unused-vars, no-undef, handle-callback-err */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify the need for these?

If they were meant to handle the callback of Lock action then I'd prefer to just either put () => {} in there or comment out the arguments if they are needed for some reason.

CheckBound: Validate(record => true)
})
/* eslint-disable no-unused-vars */
CheckBound: Validate(record => true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

() => true?

@@ -1,3 +1,4 @@
/* eslint-disable no-unused-vars, no-undef */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Maybe just put SignUp: Action((/* data, callback */) => {?

@@ -0,0 +1,27 @@
globals:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look the same as those in schemas/.eslintrc.yml, could we perhaps put them in a common config file (i.e. schemas-eslint-config.js) that would be extendsed in schemas/.eslintrc.yml and test/fixtures/.eslintrc.yml?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants