codeclimate / codeclimate-eslint

Code Climate Engine for ESLint
MIT License
95 stars 93 forks source link

Can we add support for Standard Style out-of-the-box without config files? #130

Closed deltaidea closed 8 years ago

deltaidea commented 8 years ago

Philosophy of Standard is no configuration - it should be easy and simple to enforce this style and hard to change it.

Can we have a special case where if there's "standard" in dependencies or devDependencies, then the style is enabled automatically? This can easily be determined statically, so I don't see any issues.

It is really annoying to use Standard and then still have to add .codeclimate.yml and .eslintrc config files just to get it working in a standard way.

ntwb commented 8 years ago

Standard is just a name, not an actual standard /shrug

deltaidea commented 8 years ago

That is correct, and it's even stated on their website. Which is exactly why I don't propose making it the default. I just want it working out-of-the-box when the "standard" package is in dependencies.

tunnckoCore commented 8 years ago

It's not, but it should be. It is very stable (and old, almost 2 years?) and already used by big members and projects. And it is one of the best things ever in the community.

Anyway, i not forcing to use it by default, too. But how to enable it in codeclimate? Otherwise I'm just going to drop codeclimate from my flow, because it cause me more headaches (and slows my development) than it solves in the last year or two - time that i'm using it.

deltaidea commented 8 years ago

Found this article: https://codeclimate.com/changelog/56422a5769568030c40010a6

To use one of these configurations with our ESLint Engine:

  • copy the file, adding rule overrides as needed, then
  • add `”extends”: “airbnb” to your .eslintrc

I assume this is also how it works with other supported styles. In other words, you want us to copy the whole Standard style and paste it in .eslintrc, and add "extends": "standard" to enable parser plugins.

This is confirmed in lib/validate_config.js which doesn't even consider the config valid if it doesn't contain at least one custom rule.

This doesn't sound good.

tunnckoCore commented 8 years ago

In other words, you want us to copy the whole Standard style and paste it in .eslintrc, and add "extends": "standard" to enable parser plugins.

Yea. I understand it that way too. And that's insane. If we include eslintrc we just won't use standard, but their config directly.

dblandin commented 8 years ago

Hey all,

In other words, you want us to copy the whole Standard style and paste it in .eslintrc, and add "extends": "standard" to enable parser plugins.

Sorry for the confusion here. You shouldn't have to copy/paste any rules in order to use Standard style with the ESLint engine.

The following minimal .eslintrc.js file should work just fine:

{ "extends": ["standard"] }

This will yield a valid configuration with rules from Standard style. This method of configuration comes straight from the eslint-config-standard README.

Optionally (but recommended), you can add a .codeclimate.yml file that enables the ESLint engine and targets the ESLint 3 channel:

engines:
  eslint:
    enabled: true
    channel: "eslint-3"
ratings:
  paths:
  - **/*.js

The default channel (stable) for the ESLint engine targets ESLint 1, so to use the latest and greatest, you can specify that you want to use ESLint 3 via the eslint-3 channel. Adding **/*.js to the ratings paths tells Code Climate which files should be rated and incorporated into your repository's grade.

Philosophy of Standard is no configuration - it should be easy and simple to enforce this style and hard to change it.

Standard doesn't require configuration when using it via the Standard CLI, but when using it through an integration, like the ESLint plugin, some configuration is required. I would however be interested in a separate codeclimate-standard engine that wraps Standard and relies on its own integration with ESLint. That would require no other configuration from the user beyond enabling the engine within .codeclimate.yml.

Which is exactly why I don't propose making it the default. I just want it working out-of-the-box when the "standard" package is in dependencies.

This is interesting and worth considering as an extension to the way we currently infer a Code Climate configuration when a .codeclimate.yml file is absent. Right now, we rely on regex file path patterns to decide whether an engine should be auto-enabled within an inferred configuration.

In response to your PRs: A future iteration in which we check package.json or Gemfile files to determine engine eligibility makes sense although I don't think that the logic should go into the engine itself but up a level to where the configuration is initialized.

For example, if standard is found within a user's package.json file, I can see us either auto-enabling the ESLint engine with the appropriate configuration that includes the required extends or auto-enabling a new codeclimate-standard engine that wraps the Standard CLI.

tunnckoCore commented 8 years ago

@dblandin great. I think this should be clarified in the docs.

The following minimal .eslintrc.js file should work just fine:

I thought for that, but don't want it too. Actually, thought to move to eslint + standard-config, cuz don't like 1-2 rules of the standard, but.

dblandin commented 8 years ago

I think this should be clarified in the docs.

Yep, the docs need some work, especially around plugins and channels.

Actually, thought to move to eslint + standard-config, cuz don't like 1-2 rules of the standard

In that case you should be able to add your rule overrides right after the extends declaration:

{
  "extends": ["standard"],
  "rules": {
    "no-useless-constructor": "off"
  }
}
tunnckoCore commented 8 years ago

@dblandin yea, know that, thanks.

btw, suggestion (here as offtopic, cuz can't find the repo of CodeClimate Chrome Extension): make the badge looks like Github's "star/fork/watch" labeled button and when click move to redirect as currently? I think it may look better? :)

dblandin commented 8 years ago

btw, suggestion (here as offtopic, cuz can't find the repo of CodeClimate Chrome Extension): make the badge looks like Github's "star/fork/watch" labeled button and when click move to redirect as currently? I think it may look better? :)

The browser extension isn't open source at the moment. Right now the badge simply links to the Code Climate page rather than triggering a user action on GitHub (star/fork/watch). It's designed to match other badges, but we'll certainly consider a change to make it look less out of place. Thanks for the feedback :)

tunnckoCore commented 8 years ago

simply links to the Code Climate page rather than triggering a user action on GitHub (star/fork/watch

i'm not saying it should trigger some action. Yea, just to "look in place" with github's style. :)

edit: btw, one issue: when it is "Upgrade to ..." it links to wrong place and there some undefined in the url and some token (i believe)

The browser extension isn't open source at the moment.

strange.

deltaidea commented 8 years ago

@dblandin Thank you very much for your response!

Yes, moving this to codeclimate/codeclimate makes sense. I'm going to consider helping with that. It's probably better to pass config to this engine instead of creating a new engine, as that wouldn't be DRY (managing same dependencies like eslint in multiple engines wouldn't be fun).

By the way, why not merge the three branches for different channels into master? Would it be possible to use different files (package.json, etc.) in Makefile depending on which channel we're currently building? I'm not sure how exactly the engine is built/invoked, but I can imagine it's possible to pass information to Makefile.

dblandin commented 8 years ago

Yes, moving this to codeclimate/codeclimate makes sense. I'm going to consider helping with that. It's probably better to pass config to this engine instead of creating a new engine, as that wouldn't be DRY (managing same dependencies like eslint in multiple engines wouldn't be fun).

That sounds good! In that case, would you mind closing this issue and the related PRs? Unfortunately, given the way our inferred configuration works right now, you can't rely on an absent ESLint configuration since we'll add one if no configuration is present.

By the way, why not merge the three branches for different channels into master? Would it be possible to use different files (package.json, etc.) in Makefile depending on which channel we're currently building? I'm not sure how exactly the engine is built/invoked, but I can imagine it's possible to pass information to Makefile.

Engine channels often have different dependencies and might also rely on vastly different engine codebases. For example, the stable channel of this engine (targeting ESLint 1) uses a fork of ESLint, while the eslint-2 and eslint-3 chanels monkey patch ESLint instead. Going forward, I don't think we want to require different engine channels to share a common codebase.

Additionally, we build and deploy engine updates in response to our CI pipeline. This automation is easier to manage when a given branch only maps to one engine channel.

We'll certainly be iterating on this process but that's how it works right now. :smile:

deltaidea commented 8 years ago

I see.

Just gonna gently remind to write down a todo somewhere to update the docs on this engine and common setups with shareable configs.

Thank you for your support!

lucasmazza commented 8 years ago

The following minimal .eslintrc.js file should work just fine:

{ "extends": ["standard"] }

@dblandin this works for the eslint-2 and eslint-3 channels, but looks like it doesn't work with the default image, even though it works as expected when running eslint 1.x manually. It seems that the getConfigForFile call on validate_config.js doesn't expands the inherited rules into the config object, and the engine fails saying that are not rules enabled for the config.

Would make sense to either change the validation to also check for the extends key and consider { "extends": ["standard"] } a valid config object.

zanona commented 6 years ago

Hey guys, at the moment if you head to this page: https://docs.codeclimate.com/docs/eslint

You will see that eslint-config-standard and standard are listed as ESLint plugins. This is very confusing as I would indeed think about the extends field under .eslintrc for eslint-config-standard but how about standard? How do we use this? Are we talking about standard CLI here? — Because that's where the link points to.

😕

davehenton commented 6 years ago

@chrishulton Can you take a look and give us some guidance on this most recent comment?

chrishulton commented 6 years ago

Hm, I believe the docs are incorrect in linking to standard separately, as we are just including the eslint-config version in this engine.

As mentioned earlier in this issue, that would be configured along the lines of:

{
  "extends": ["standard"],
  "rules": {
    "no-useless-constructor": "off"
  }
}
zanona commented 6 years ago

Thanks, @chrishulton, perhaps updating the docs would help in this case.

davehenton commented 6 years ago

Thanks @chrishulton

I've updated our doc here to help eliminate confusion around this. Let me know if find any other references to this that should be updated! Thanks for your time and patience with this @zanona.