Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Make eslint object-curly-spacing rule enforced #807

Merged
merged 1 commit into from
Nov 16, 2016

Conversation

amyrlam
Copy link
Contributor

@amyrlam amyrlam commented Nov 16, 2016

Fixes: part of #732

Relates to: #770 cc @btecu

Changes proposed in this pull request:

Why? I just realized my fork was terribly out of date when I started looking at #732 recently. I have previously used ember-suave, am just getting started with eslint.

If this PR is ok:

If I'm misunderstanding something please let me know!

cc @HospitalRun/core-maintainers

Re: http://eslint.org/docs/rules/object-curly-spacing

Realized my fork was out of date when started looking at this issue recently. I have previously used ember-suave, just getting started with eslint. 

Since many of the rules outlined in HospitalRun#732 aren’t part of eslint recommended https://github.com/DockYard/eslint-plugin-ember-suave/blob/master/config/recommended.js, I suggest adding the rules to the eslintrc.js so that they are enforced as they are fixed.

If this PR is ok:
- I’ll add the rest of the already completed rules from HospitalRun#732 in a follow-on PR.
- I will go through the remaining rules from HospitalRun#732, and add the rule to the .eslintrc.js as the rules are fixed.
@amyrlam
Copy link
Contributor Author

amyrlam commented Nov 16, 2016

Not sure why Travis failed since I can run npm test locally...any debugging ideas, please ping me here or amyrlam on the HospitalRun Slack!

@jkleinsc
Copy link
Member

@amyrlam thanks for the PR!

I'm in favor of updating .eslintrc.js as things are fixed.

In regards to Travis failing, for some reason sometimes Travis fails (not sure why?) and I have to manually restart the build/test. I restarted the test and it now passed.

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jkleinsc jkleinsc merged commit daaec08 into HospitalRun:master Nov 16, 2016
@amyrlam
Copy link
Contributor Author

amyrlam commented Nov 16, 2016

Ah thanks for the restart. Will start on the others!

@amyrlam amyrlam deleted the amyrlam/object-curly-spacing branch November 16, 2016 18:42
@btecu
Copy link
Contributor

btecu commented Nov 16, 2016

@amyrlam What we should be doing is finishing #732 and once that's completed, turn on ember-suave.

@amyrlam
Copy link
Contributor Author

amyrlam commented Nov 16, 2016

@btecu The rules that you started in the PR's linked in #732 aren't enforced unless you list them. See: http://eslint.org/docs/rules/, object-curly-spacing is not enforced as part of "extends": "eslint:recommended".

Totally agree we should add plugin:ember-suave/recommended after getting through the eslint ones re: https://github.com/DockYard/eslint-plugin-ember-suave#configuration.

If you want to work on this together feel free to ping me on the HospitalRun Slack!

@btecu
Copy link
Contributor

btecu commented Nov 16, 2016

@amyrlam That's not correct. They are enforced in ember-suave:
https://github.com/DockYard/eslint-plugin-ember-suave/blob/master/config/base.js#L34

So once #732 is completed we'll have to remove the one you just add because it will be covered by ember-suave.

@amyrlam
Copy link
Contributor Author

amyrlam commented Nov 16, 2016

That makes sense, once suave is added remove the duplicate rules. But for now this will keep the rules enforced and keeps the tracking in the code.

Or, we could add suave now and override any rules that haven't been fixed yet.

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.

3 participants