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

Update: Use sails-hook-lint for Sails integration #7679

Merged
merged 2 commits into from Dec 1, 2016
Merged

Update: Use sails-hook-lint for Sails integration #7679

merged 2 commits into from Dec 1, 2016

Conversation

amorriscode
Copy link
Contributor

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.

After further testing, sails-hook-lint has been determined to be the best Sails.js integration available.
@mention-bot
Copy link

@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.

@eslintbot
Copy link

LGTM

Copy link
Member

@kaicataldo kaicataldo left a 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.

@amorriscode
Copy link
Contributor Author

@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.

@nzakas nzakas added documentation Relates to ESLint's documentation evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 1, 2016
Copy link
Member

@nzakas nzakas left a 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.

@amorriscode
Copy link
Contributor Author

@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.

@nzakas
Copy link
Member

nzakas commented Dec 1, 2016

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.
@eslintbot
Copy link

LGTM

@amorriscode
Copy link
Contributor Author

amorriscode commented Dec 1, 2016

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!

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 1, 2016
Copy link
Member

@kaicataldo kaicataldo left a 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!

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@kaicataldo
Copy link
Member

Going to go ahead and merge this, @nzakas, since your concern was addressed

@kaicataldo kaicataldo merged commit f80c278 into eslint:master Dec 1, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants