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

ESLint: enabled no-trailing-spaces and eol-last rules #11998

Merged
merged 1 commit into from
Dec 14, 2015

Conversation

ficristo
Copy link
Collaborator

I wanted to enable the indent rule but it seemed to be influenced by the trailing whitespaces.
So I've enabled no-trailing-spaces rule and, since there were only a few warnings the eol-last rule too.
Sorry for the big PR but it's only whitespaces changes and so there should not be any change in behaviour.

@abose @zaggino This PR can bitrot easily: I appreciate a fast turnaround.

@zaggino
Copy link
Contributor

zaggino commented Dec 12, 2015

LGTM from me

@petetnt
Copy link
Collaborator

petetnt commented Dec 13, 2015

👍 from me too

zaggino added a commit that referenced this pull request Dec 14, 2015
ESLint: enabled no-trailing-spaces and eol-last rules
@zaggino zaggino merged commit 2a832d0 into adobe:master Dec 14, 2015
@ficristo
Copy link
Collaborator Author

@zaggino @petetnt Thank you!

@ficristo ficristo deleted the eslint-trailing-eol branch December 14, 2015 09:01
@abose
Copy link
Contributor

abose commented Dec 14, 2015

@ficristo @zaggino As part of PR https://github.com/adobe/brackets/pull/11693/files
.brackets.json file was edited from "linting.prefer": ["JSLint", "JSHint"], to "linting.prefer": ["ESLint"],
This has caused JSline to stop working from brackets code base when editing in brackets editor!
As we have not yet integrated eslint into the lint panel in brackets, brackets does not lint JS files now.

@ficristo
Copy link
Collaborator Author

@abose You are right... I'm using the eslint plugin so I didn't saw it...
Meanwhile you can use grunt eslint which run already on travis.
For now it's a bit annoying, but #11988 should fix this.
Waiting that PR is merged you could:

  • reverting the two PR of ESLint: this seems more work than what it is worth
  • reintroducing JSLint: this could interfere with ESLint rules
    What do you think?

@abose
Copy link
Contributor

abose commented Dec 14, 2015

We can add eslint also to the preferred linters for the time being. And have #11988 just have eslint alone. That would be a good transition.

@ficristo
Copy link
Collaborator Author

We can add eslint also to the preferred linters for the time being

I'm not sure to have understood: do you mean changing the config to this: "linting.prefer": ["ESLint", "JSLint"] ?

@abose
Copy link
Contributor

abose commented Dec 14, 2015

I was thinking of something like that but now figured it doesn't work like that.
What will happen if we just have JSLint here for the time being till #11988 is merged?

@ficristo
Copy link
Collaborator Author

When I written the PR to integrate ESLint I tried to use similar rules to the ones already there.
Using JSLint shouldn't create problems but there is a chance that they could interfere: JSLint could warn about something that's fine for ESLint or worse could not warn something that's not fine for ESLint.

@abose
Copy link
Contributor

abose commented Dec 14, 2015

What should we do in this case then?

@zaggino
Copy link
Contributor

zaggino commented Dec 14, 2015

I've merged @ficristo rollback PR and I'll add these changes to #11988

@peterflynn
Copy link
Member

@abose @zaggino @ficristo Sorry for not speaking up sooner, but I have serious concerns about this change. Brackets itself (and CodeMirror in general) automatically generates whitespace on blank lines based on indent. This ESLint rule means we will be constantly fighting with Brackets's own output, which seems like a bad policy.

I would strongly suggest we change the ESLint setting to "no-trailing-spaces": [2, { "skipBlankLines": true }] so that we don't have that problem.

If I'd noticed this sooner I would have also suggested reverting all the changes that remove whitespace on blank lines, so that we don't have verbose diffs where people editing the code gradually start reintroducing all that indent whitespace... but I guess it's too late for that now :-/

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

Successfully merging this pull request may close these issues.

5 participants