[eslint] merge custom rules into a single plugin #33733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spalger
added
review
Team:Operations
|
Pinging @elastic/kibana-operations |
This comment has been minimized.
This comment has been minimized.
mshustov
reviewed
Mar 22, 2019
| @@ -21,5 +21,6 @@ module.exports = { | |||
| rules: { | |||
| 'require-license-header': require('./rules/require_license_header'), | |||
| 'disallow-license-headers': require('./rules/disallow_license_headers'), | |||
| 'no-default-export': require('./rules/no_default_export'), | |||
There was a problem hiding this comment.
overall +1 from me for the PR, because today I spent some time trying to add another custom rule while porting #33562
just curious - why we use custom no_default_export rule instead of https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-default-export.md ?
our doesn't handle export { foo as default }; for example
There was a problem hiding this comment.
Probably because it didn't exist at the time, but I'm not sure. I'd love to review a PR that moves to the rule from eslint-plugin-import!
spalger
pushed a commit
to spalger/kibana
that referenced
this pull request
Mar 22, 2019
…3739) The fixtures used by reporting's `extract()` tests have tripped me and others up several times because from the outside they look and behave like source files, but require updating archive fixtures whenever the source is touched. The source of this file is irrelevant though, since the tests are just making sure that the extraction is accurate. This PR updates the fixture to a markdown file with Lorem Ipsum in it, so it won't look relevant to people making broad strokes changes to files like elastic#33733
spalger
pushed a commit
that referenced
this pull request
Mar 22, 2019
The fixtures used by reporting's `extract()` tests have tripped me and others up several times because from the outside they look and behave like source files, but require updating archive fixtures whenever the source is touched. The source of this file is irrelevant though, since the tests are just making sure that the extraction is accurate. This PR updates the fixture to a markdown file with Lorem Ipsum in it, so it won't look relevant to people making broad strokes changes to files like #33733
spalger
pushed a commit
to spalger/kibana
that referenced
this pull request
Mar 22, 2019
…3739) The fixtures used by reporting's `extract()` tests have tripped me and others up several times because from the outside they look and behave like source files, but require updating archive fixtures whenever the source is touched. The source of this file is irrelevant though, since the tests are just making sure that the extraction is accurate. This PR updates the fixture to a markdown file with Lorem Ipsum in it, so it won't look relevant to people making broad strokes changes to files like elastic#33733
💚 Build Succeeded |
spalger
force-pushed
the
refactor/eslint-plugins
branch
from
aa5e873
to
da05a03
Compare
tylersmalley
approved these changes
Mar 22, 2019
💚 Build Succeeded |
spalger
pushed a commit
to spalger/kibana
that referenced
this pull request
Mar 23, 2019
I'd like to add another custom eslint rule, but there isn't a very good place to do that right now. We have the `eslint-plugin-kibana-custom` package, which is super simple but isn't in the `@kbn` namespace and isn't included in the root eslint config, and `@kbn/eslint-plugin-license-header` is too specific, so I've merged those two packages into `@kbn/eslint-plugin-eslint`, which is a little redundant but allows is to refer to the rules within it as `@kbn/eslint/{rule}`, which feels nice.
Thoughts?
_**NOTE:**_ merging the eslint rules from the two packages means enabling prettier for the code from `@kbn/eslint-plugin-license-header`, all those changes are made in elastic@42c7da6. [View the changes without the prettier updates](elastic/kibana@b647f2b...74e07a0)
spalger
pushed a commit
to spalger/kibana
that referenced
this pull request
Mar 23, 2019
I'd like to add another custom eslint rule, but there isn't a very good place to do that right now. We have the `eslint-plugin-kibana-custom` package, which is super simple but isn't in the `@kbn` namespace and isn't included in the root eslint config, and `@kbn/eslint-plugin-license-header` is too specific, so I've merged those two packages into `@kbn/eslint-plugin-eslint`, which is a little redundant but allows is to refer to the rules within it as `@kbn/eslint/{rule}`, which feels nice.
Thoughts?
_**NOTE:**_ merging the eslint rules from the two packages means enabling prettier for the code from `@kbn/eslint-plugin-license-header`, all those changes are made in elastic@42c7da6. [View the changes without the prettier updates](elastic/kibana@b647f2b...74e07a0)
spalger
pushed a commit
that referenced
this pull request
Mar 23, 2019
spalger
pushed a commit
that referenced
this pull request
Mar 23, 2019
joelgriffith
pushed a commit
that referenced
this pull request
Mar 27, 2019
The fixtures used by reporting's `extract()` tests have tripped me and others up several times because from the outside they look and behave like source files, but require updating archive fixtures whenever the source is touched. The source of this file is irrelevant though, since the tests are just making sure that the extraction is accurate. This PR updates the fixture to a markdown file with Lorem Ipsum in it, so it won't look relevant to people making broad strokes changes to files like #33733
joelgriffith
pushed a commit
that referenced
this pull request
Mar 27, 2019
I'd like to add another custom eslint rule, but there isn't a very good place to do that right now. We have the `eslint-plugin-kibana-custom` package, which is super simple but isn't in the `@kbn` namespace and isn't included in the root eslint config, and `@kbn/eslint-plugin-license-header` is too specific, so I've merged those two packages into `@kbn/eslint-plugin-eslint`, which is a little redundant but allows is to refer to the rules within it as `@kbn/eslint/{rule}`, which feels nice.
Thoughts?
_**NOTE:**_ merging the eslint rules from the two packages means enabling prettier for the code from `@kbn/eslint-plugin-license-header`, all those changes are made in 42c7da6. [View the changes without the prettier updates](b647f2b...74e07a0)
spalger
added
the
release_note:skip
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'd like to add another custom eslint rule, but there isn't a very good place to do that right now. We have the
eslint-plugin-kibana-custompackage, which is super simple but isn't in the@kbnnamespace and isn't included in the root eslint config, and@kbn/eslint-plugin-license-headeris too specific, so I've merged those two packages into@kbn/eslint-plugin-eslint, which is a little redundant but allows is to refer to the rules within it as@kbn/eslint/{rule}, which feels nice.Thoughts?
NOTE: merging the eslint rules from the two packages means enabling prettier for the code from
@kbn/eslint-plugin-license-header, all those changes are made in 42c7da6. View the changes without the prettier updates