Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Eslint #11693

Merged
merged 1 commit into from
Dec 8, 2015
Merged

Eslint #11693

merged 1 commit into from
Dec 8, 2015

Conversation

ficristo
Copy link
Collaborator

I added support for ESLint in grunt and removed JSHint.
I need to remove all the JSLint comments from the source files.
While I tried to preserve the old behavior (actually I want some more rules),
I need to fix some rules (the one with 0 in the eslintrc file).
And to decide what to do with the default linter in Brackets.
Personally I am in favor to remove it completely, people can use the extensions instead.
What you all think?

@abose
Copy link
Contributor

abose commented Sep 13, 2015

Why do you prefer eslint over jshint for use with brackets?

@petetnt
Copy link
Collaborator

petetnt commented Sep 13, 2015

@abose see this discussion about replacing JSLint in #11644

@abose
Copy link
Contributor

abose commented Sep 13, 2015

got it!
@ficristo did you do a grunt build to see if there are any build failures due to eslint problems? There would be jslint annotations listed all over the code base. Is it compatible with eslint annotations?

@ficristo
Copy link
Collaborator Author

@abose I added ESLint because I thought was the preferred one by the Brackets community. As pointed out by @petetnt I saw in some issues / forum that was the preferred choice.
If you use JSHint and still want stylistic checks you should use JSCS too.
I run grunt eslint with the set of rules I written and it passes. As I written in the first post there are some rule disabled.
I tried grunt build too and I didn't noticed any problem with JSLint.
I didn't try inline comments, but personally I want to remove all of them.

@ingorichter ingorichter mentioned this pull request Sep 14, 2015
7 tasks
@IanVS
Copy link

IanVS commented Sep 14, 2015

@ficristo You may want to consider adding "extends": "eslint:recommended" to your .eslintrc, since as of version 1.0.0, all rules are disabled by default (see http://eslint.org/docs/user-guide/migrating-to-1.0.0). Recommended rules are denoted by "(recommended)" in http://eslint.org/docs/rules/.

@@ -20,7 +20,7 @@
"extension_url": "https://s3.amazonaws.com/extend.brackets/{0}/{0}-{1}.zip",
"linting.enabled_by_default": true,
"build_timestamp": "",
"healthDataServerURL" : "https://healthdev.brackets.io/healthDataLog"
"healthDataServerURL": "https://health.brackets.io/healthDataLog"
Copy link
Contributor

Choose a reason for hiding this comment

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

this line shouldn't have changed I guess

@ficristo
Copy link
Collaborator Author

@IanVS I tried "extends": "eslint:recommended" but there are still too many errors, and I think to prefer to list all the rules.

I would like to land the patch as is and file followup to enable (or remove) the current set of rule.
Eventually we also can add other rules.

@abose abose added this to the Release 1.6 milestone Sep 14, 2015
@abose
Copy link
Contributor

abose commented Sep 14, 2015

Moved to milestone 1.6 so that we could work on comprehensive support of es6 related pr's.

@le717
Copy link
Contributor

le717 commented Sep 16, 2015

Just chiming in here to say a while back I began rewriting the JSHint rules for ESLint (+ incorporating the style guide wiki page). If it helps any, here are set of rules I had already implemented. The Brackets source failed heavily against them (partially from incomplete rule porting) but it was working.

@zaggino
Copy link
Contributor

zaggino commented Sep 16, 2015

hi @le717 , how does nodeca/indent differ from eslint's indent rule?

@petetnt
Copy link
Collaborator

petetnt commented Sep 17, 2015

@zaggino ESLint didn't get indendation support until version 0.14 so the plugin was neccessary. It can safely be replaced by native indent now though.

@le717
Copy link
Contributor

le717 commented Sep 18, 2015

@zaggino I believe @petetnt is correct. IIRC, ESLint did not have native intent support, so I added that plugin to enforce it (as it is not already being done and manually checking it on code reviews when it could be automatic is not the wisest idea).

@zaggino
Copy link
Contributor

zaggino commented Sep 18, 2015

yes but eslint is now pass 1.0 and has indent support so we should use that one instead of a plugin

@le717
Copy link
Contributor

le717 commented Sep 19, 2015

@zaggino Yes, it would be best to use native indent support. :)

to lint Brackets core code.
@ficristo
Copy link
Collaborator Author

Is there a way I can help to move forward this?

@abose
Copy link
Contributor

abose commented Nov 28, 2015

tagging @swmitra

@zaggino
Copy link
Contributor

zaggino commented Dec 8, 2015

the PR seems good, is there any reason not to merge this?

@petetnt
Copy link
Collaborator

petetnt commented Dec 8, 2015

LGTM 👍

abose added a commit that referenced this pull request Dec 8, 2015
@abose abose merged commit 79f3e2c into adobe:master Dec 8, 2015
@abose
Copy link
Contributor

abose commented Dec 8, 2015

Verified build works. Merging.

@abose
Copy link
Contributor

abose commented Dec 8, 2015

Thanks @ficristo
Should ESlint be prefferd for the lint panel in brackets?

@zaggino
Copy link
Contributor

zaggino commented Dec 8, 2015

@abose Yes, I believe so

@petetnt
Copy link
Collaborator

petetnt commented Dec 8, 2015

@abose That would require need integration of the ESLint (similarly how JSLint is integrated now under https://github.com/adobe/brackets/tree/d939740fd5c368c347ac6a8c7cef2941981aa2fb/src/extensions/default IMO

@zaggino has the great ESLint extension so I think he has the most insight on how much work it would require. Personally I would love it 💐

@zaggino
Copy link
Contributor

zaggino commented Dec 8, 2015

It's easy to get eslint integrated direclty into Brackets core, I can do a PR for this if you want guys.

@abose
Copy link
Contributor

abose commented Dec 8, 2015

+1 for ESLint in default extensions in core.

@ficristo
Copy link
Collaborator Author

ficristo commented Dec 8, 2015

I agree to switch from JSLint to ESLint as default linter in core.

@ficristo ficristo deleted the eslint branch December 8, 2015 10:49
@abose
Copy link
Contributor

abose commented Dec 8, 2015

one part of #11644 will also be addressed by the switch.

@petetnt petetnt mentioned this pull request Dec 8, 2015
3 tasks
@petetnt
Copy link
Collaborator

petetnt commented Dec 8, 2015

I created #11984 to track the remaining steps of the ESLint integration

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

Successfully merging this pull request may close these issues.

6 participants