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

[eslint-config-kibana] Upgrade eslint-config to 0.10.0. #13323

Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Aug 3, 2017

@simianhacker Would you mind going through the TSVB UI and making sure I haven't broken anything? I had to change a bunch of components to named exports, and I'm not sure I updated all of the import call sites to match.

@cjcenizal cjcenizal added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc chore labels Aug 3, 2017
@cjcenizal cjcenizal requested a review from spalger August 3, 2017 18:16
import AggSelect from './agg_select';
import AggRow from './agg_row';
import createChangeHandler from '../lib/create_change_handler';
import createSelectHandler from '../lib/create_select_handler';
import createTextHandler from '../lib/create_text_handler';

class FilterRatioAgg extends Component {
export const Static = props => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this component because I think the original name was a typo.

import AggSelect from './agg_select';
import MetricSelect from './metric_select';
import AggRow from './agg_row';
import createChangeHandler from '../lib/create_change_handler';
import createSelectHandler from '../lib/create_select_handler';
import createTextHandler from '../lib/create_text_handler';

class DerivativeAgg extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing these components from classes to function makes a lot more changed lines than necessary. Is this a requirement of the new eslint rules? Is it autofixable? If there are auto-fixed changes and manual changes, it would be great if they were in separate commits so they could be reviewed with different scrutiny.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the class -> function conversion isn't necessary, I'd be fine with it still but another separate commit would be very helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preferring stateless functional components is one of the rules I added, but no it isn't autofixable unfortunately. It also means moving a lot of default exports to named exports (which is also part of our style guide, I believe). I broke this PR up into 2 commits (automatic fixes and manual ones), for your convenience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks!

@cjcenizal cjcenizal force-pushed the chore/upgrade-eslint-config-0.10.0 branch from 2e1ad7e to e422a3b Compare August 3, 2017 22:00
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasneirynck thomasneirynck self-requested a review August 9, 2017 18:19
Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

PR's like this are practically un-review-able and quite frankly pretty risky based on the size alone. I would rather see you create 11 PR's for each new rule and the corresponding refactor then one massive PR like this. Another approach would be to try and isolate the reactor to logical components so they are not all jumbled up into one gigantic PR.

@cjcenizal
Copy link
Contributor Author

@simianhacker I completely understand + agree. I think I can provide some guidance on this PR which will help you review it in its current form.

The following rules had no violations:

The first commit contains autofixes -- do you want to review these? I'm not sure these can be affected by code review, unless you want to challenge a rule we've adopted. If you look at the commit with ?w=1 appended to ignore whitespace changes, there's very little to review: 595d37f?w=1. Here are the rules which have been applied:

The second commit consists of manual fixes, but if you look at the commit with whitespace ignored, it's also a lot easier to follow: e422a3b?w=1. Here are the rules I applied:

The prefer-stateless-function rule incurred the most changes, but they involved a straightforward pattern:

  • Convert the component into a stateless function, which involved a large whitespace change
  • Convert the default export to a named export (which is the part I'm most concerned about, and need your help with)

Does this help?

@thomasneirynck thomasneirynck removed their request for review August 9, 2017 20:58
@simianhacker
Copy link
Member

I reviewed this to the best of my abilities... reviewed the code and tested (manually) all the aggs that where refactored) LGETM

@cjcenizal
Copy link
Contributor Author

Thanks @simianhacker !

@cjcenizal cjcenizal force-pushed the chore/upgrade-eslint-config-0.10.0 branch from e422a3b to 6a842f5 Compare August 11, 2017 17:24
@cjcenizal cjcenizal merged commit fbaf4e6 into elastic:master Aug 11, 2017
@cjcenizal cjcenizal deleted the chore/upgrade-eslint-config-0.10.0 branch August 11, 2017 18:48
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 11, 2017
* Upgrade eslint-config to 0.10.0.
* Fix linting violations with popover and typography stuff in UI Framework.
cjcenizal added a commit that referenced this pull request Aug 11, 2017
)

* Upgrade eslint-config to 0.10.0.
* Fix linting violations with popover and typography stuff in UI Framework.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.1.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants