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

[tslint] Fix violations in kbn-pm #19335

Merged
merged 11 commits into from
May 23, 2018
Merged

Conversation

spalger
Copy link
Contributor

@spalger spalger commented May 22, 2018

#19105 included overrides for several tslint rules that the existing TypeScript in kbn-pm violated, in order to keep the size of that PR smaller.

This PR removes those overrides, autofixes the violations where possible, and then manually fixes the remaining violations.

I suggest reviewing this PR commit-by-commit rather than all at once.

@spalger spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.4.0 labels May 22, 2018
@spalger spalger requested a review from azasypkin May 22, 2018 23:14
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -11,6 +11,7 @@ function help() {
.map(commandName => commands[commandName])
.map(command => `${command.name} - ${command.description}`);

/* tslint:disable-next-line no-console */
console.log(dedent`
Copy link
Member

@azasypkin azasypkin May 23, 2018

Choose a reason for hiding this comment

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

question: can't we just create log.ts that exposes function with with console.log signature and add this annotation only there or just keep tslint.yaml with no-console: false? Just don't want to embrace these /* tslint: something */ all over the place, I'd rather have this somewhere in a single place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I hope most people are averse to the tslint:disable directives and will avoid console.log() usage because of it, but I agree that having a suitable replacement is better than spreading the directives all over kbn-pm

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger merged commit ce20463 into elastic:master May 23, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request May 23, 2018
* [tslint/kbn-pm] apply autofixes

* [tslint/kbn-pm] whitelist all console.log calls for now

* [tslint/kbn-pm] sort object keys alphabetically

* [tslint/kbn-pm] use comments to "fill" empty blocks

* [tslint/kbn-pm] use I-prefix for interface names

* [tslint/kbn-pm] use arrow-functions where possible

* [tslint/kbn-pm] use lowerCamelCase for variable names

* [tslint/kbn-pm] prevent shadowed variable names

* [tslint/kbn-pm] avoid variable name and type clashes

* [tslint/kbn-pm] replace console.log statements with bare-bones logger
spalger pushed a commit that referenced this pull request May 23, 2018
Backports the following commits to 6.x:
 - [tslint] Fix violations in kbn-pm (#19335) (ce20463)
@spalger
Copy link
Contributor Author

spalger commented May 23, 2018

6.4/6.x: 09ba816

@spalger spalger deleted the fix/kbn-pm/tslint branch May 23, 2018 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants