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 configs and recommendations #146

Merged
merged 9 commits into from
Nov 15, 2017
Merged

Update configs and recommendations #146

merged 9 commits into from
Nov 15, 2017

Conversation

michalsnik
Copy link
Member

This PR:

  • updates base and recommended configs
  • removes default eslint rules' settings, so this plugin will no longer interfere with other plugins' settings
  • enables experimentalObjectRestSpread feature in eslint parser
  • regroups all rules using well known common categories: Best Practices, Possible Errors, Stylistic Issues
  • updates script for generating tables in readme with Deprecated Rules section.
  • updates all rules' recommendations

I hope this PR is a good starting point for an elaborate discussion which rules should be recommended, and which deprecated.

If you have any propositions/suggestions, also regarding categories to which specific rules are assigned to, please post a comment below :)

cc @rwwagner90 @Turbo87 @jbandura

PS. This PR will be probably open for at least month or more, as it will require us to do another major release after merge. I don't want to rush it, so in the meanwhile we can continue working on new rules/fixing current ones.

@@ -71,73 +71,56 @@ The `--fix` option on the command line automatically fixes problems reported by

<!--RULES_TABLE_START-->
Copy link
Member

Choose a reason for hiding this comment

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

the sentence above "All rules below with a check mark ✅ are enabled by default while using plugn:ember/base or plugin:ember/recommended configs." needs to be adjusted

@Turbo87
Copy link
Member

Turbo87 commented Sep 26, 2017

some comments from my side:

since the release will likely be after the 2.16 release of Ember and that release will introduce the "New Module Imports" as the default we should probably enable the corresponding rules here and put some comments in the docs on how to disable them if you're on older Ember versions. Specifically this means:

  • enable new-module-imports
  • enable no-old-shims
  • enable no-global-jquery (import $ from 'jquery' is provided by New Module Imports and old shims AFAIK)
  • add a link to the codemod to the new-module-imports rule docs (as a replacement for --fix not working here)

Aside from that I have two more things I would like to change:

  • disable use-ember-get-and-set rule

    Our tutorials, guide and API docs all use this.get() and not get(this) so we should stay consistent with the rest of the ecosystem; also it's IMHO much easier to read

  • disable order-in-* rules

    they can be nice to have, but in my experience they are causing too many false positives and until --fix works for them they are unfortunately just annoying most of the time

@RobbieTheWagner
Copy link
Contributor

I would love to see use-ember-get-and-set not be the default as well, as I also prefer this.get, but I want to stay consistent with what is recommended here 😄

@RobbieTheWagner
Copy link
Contributor

@Turbo87 what false positives do you see with order-in-*? They have always worked great for me.

@Turbo87
Copy link
Member

Turbo87 commented Sep 26, 2017

@rwwagner90 e.g. empty default hooks like onSelect() {}

@RobbieTheWagner
Copy link
Contributor

@Turbo87 firstly, why define an empty hook? And secondly, didn't I see @michalsnik added support for empty method ordering now?

@jbandura
Copy link
Collaborator

jbandura commented Sep 27, 2017

nice job @michalsnik !

As I see it we should also probably consider switching off the alias-model-in-controller rule, as it's not able to detect whether we actually have any kind of model in the first place. I do see the value this rule brings, but currently in many cases this rule is just not smart enough :( If user sees a legitimate use case he can always switch it back on.

Apart from that I do agree with @Turbo87 about disabling use-ember-get-and-set rule by default, or making it not report errors for object context (i.e using this.get, this.set, etc. should be allowed).

@sandstrom
Copy link

Great that we're dropping plugin:ember/recommended and only provide plugin:ember/base (or whatever we end up calling it).

The non-ember rules creates confusion, people don't know were they're coming from and its opinionated outside the realm of Ember (no-underscore-dangle on/off does not matter for an Ember app). Will also make the readme cleaner if we don't need to explain the difference between recommended and base. Awesome! ⭐️

@michalsnik
Copy link
Member Author

I addressed all suggestions, but I think order-in-* rules prove to be really helpful and are a quick-win to keep the code well organised from the beginning. But I decided to update order-in-components rule and put empty-method below property, as I think this is clearly a better place for them.

Let me also know guys what do you think about assignments to groups :)

@RobbieTheWagner
Copy link
Contributor

I agree that order-in-* should be on. It gives a lot of value and I'm not sure why you would want to define empty lifecycle hooks, and even if you did, it should still work right?

@michalsnik
Copy link
Member Author

michalsnik commented Sep 28, 2017

You want to write empty hooks when you're creating an addon, or if you just want to put a clear information what can be passed down to a component without using ember prop types addon @rwwagner90 Currently empty methods were detected as custom methods, thus this rule wanted them to be put below actions hash, which you'd rather would not.

and we're talking about empty methods (custom ones), not empty lifecycle hooks :)

@Turbo87
Copy link
Member

Turbo87 commented Sep 30, 2017

as I mentioned above: as long as order-in-* does not support --fix they are mostly just annoying instead of helping when enabled on an older codebase that didn't follow those patterns from the start. due to that fact I'm very strongly against enabling those rules by default.

the other reason is that the order is largely a stylistic preference but not actually catching any bugs or reporting about deprecated code logic which should be the main aim of the rules in this repo.

@RobbieTheWagner
Copy link
Contributor

@Turbo87 I think the ordering should not be a preference though. With no imposed order, it's very cumbersome to switch between codebases. If you just put random things in no order throughout a 1000+ line JS file, you would have a very hard time finding everything. I think there is great value in having a standard here, so Ember developers can switch easily between projects and expect a general ordering and more easily be productive out of the box.

People could always disable it, if they don't want it, or configure it custom and do order of one thing at a time.

@Turbo87 Turbo87 changed the base branch from master to next November 15, 2017 07:45
@Turbo87
Copy link
Member

Turbo87 commented Nov 15, 2017

To get the ball rolling on this again I've created a next branch in this repo that prepares for the v5.0.0 release, I will merge this PR into it and then create a few more PRs on top of it.

@Turbo87 Turbo87 merged commit 042b134 into next Nov 15, 2017
@Turbo87 Turbo87 deleted the update-configs branch November 15, 2017 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants