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
Update: Use sails-hook-lint for Sails integration #7679
Conversation
After further testing, sails-hook-lint has been determined to be the best Sails.js integration available.
@amorriscode, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @vitorbal and @Devinsuit to be potential reviewers. |
LGTM |
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.
Mind sharing some info on why this change should be made? Is this the package recommended by the Sails.js team? I did some Googling and wasn't able to find anything.
@kaicataldo Hey Kai, the issue was the old suggested hook (sails-hook-eslint) used an extremely old version of ESLint so it wasn't very useful. Now the current npm package in the docs (sails-eslint) actually has bugs out of the box. After testing I found that sails-hook-lint provides the functionality needed and runs great out of the box. |
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.
sails-eslint was updated two months ago, sails-hook-eslint was udpated two years ago. I don't think this is a good replacement. Unless the Sails team has officially recommended sails-hook-eslint, then I don't think we should make this change.
@nzakas Hey Nicolas, you're actually looking at the wrong npm package. I suggested sails-hook-lint not sails-hook-eslint. The name is very close but is actually different. |
Oops, right you are. I still don't think we should just replace what we have now with something else that seems to be equally well-maintained. I'd rather include both - ESLint doesn't want to play favorites on something we don't understand that well. Otherwise we'd be stuck going back and forth for every framework depending on who the last person to submit a documentation update was. |
Added sails-eslint back in because there are two maintained ESLint packages for Sails.js and it is unclear which one is best.
LGTM |
I added sails-eslint back in as per your request. That is a great point that I didn't think of, I just had issues getting sails-eslint running which is why I proposed the original change. Sorry for the confusion! |
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.
LGTM. Thanks for contributing to ESLint!
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.
LGTM. Thanks
Going to go ahead and merge this, @nzakas, since your concern was addressed |
What is the purpose of this pull request? (put an "X" next to item)
[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
After further testing, sails-hook-lint has been determined to be the best Sails.js integration available.