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

Adding "setting up prettier" in CONTRIBUTING.md #46310

Merged
merged 8 commits into from
Oct 14, 2019

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Sep 21, 2019

Summary

In #16514, prettier was set up for Kibana. And styleguide recommends to use it, but prettier wasn't mentioned in CONTRIBUTING.md.

So, I've added some explanation and setup guide.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jbudz jbudz added the Team:Operations Team label for Operations Team label Sep 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations

CONTRIBUTING.md Outdated Show resolved Hide resolved
@jbudz
Copy link
Member

jbudz commented Sep 23, 2019

Thanks @sainthkh! small comment above

CONTRIBUTING.md Outdated Show resolved Hide resolved
@sainthkh
Copy link
Contributor Author

sainthkh commented Sep 23, 2019

Removed contents about prettier.

Still, I have a question about prettier:

It is not wrong to install 'prettier' plugin on VS Code and use them whenever file is saved. Right? I guess .prettierrc wouldn't exist if it should be avoided.

I became curious about this because currently eslint doesn't check no trailing comma as an error but .prettierrc automatically adds it.

@jbudz jbudz added release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0 labels Sep 23, 2019
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-Authored-By: Jonathan Budzenski <jon@budzenski.me>
@jbudz
Copy link
Member

jbudz commented Sep 23, 2019

I wouldn't recommend using the prettierrc plugin in vscode - it will read the prettierrc but we have overrides in places that eslint will manage differently.

roughly the workflow is

eslint config + prettier - (rules where eslint overrides prettier)

@jbudz
Copy link
Member

jbudz commented Sep 23, 2019

@elasticmachine merge upstream

@jbudz
Copy link
Member

jbudz commented Sep 23, 2019

nice, thanks @sainthkh.

retest

@sainthkh
Copy link
Contributor Author

OK. I'll turn prettier plugin off. Thanks.

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, maybe it's best to explicitly mention that the prettier plugin shouldn't be used in Kibana. Primarily because not all files are supposed to be prettierified and that configuration is in the eslint config not in a .prettierignore file or something (so the prettier plugin applies to too many files).

@sainthkh
Copy link
Contributor Author

@spalger I agree. So, I've added a warning to the VS code setting section.

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

@tylersmalley
Copy link
Contributor

retest

@elasticmachine

This comment has been minimized.

@spalger
Copy link
Contributor

spalger commented Sep 26, 2019

@elasticmachine merge upstream

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

@elasticmachine merge upstream - sorry for all the pings, jenkins went down this time. one more time, I'll override if we're still red

@jbudz

This comment has been minimized.

@tylersmalley
Copy link
Contributor

tylersmalley commented Oct 14, 2019

@elasticmachine merge upstream

@tylersmalley tylersmalley merged commit c87b761 into elastic:master Oct 14, 2019
@tylersmalley
Copy link
Contributor

Merging since this is docs.

tylersmalley pushed a commit that referenced this pull request Oct 14, 2019
Co-Authored-By: Jonathan Budzenski <jon@budzenski.me>
@tylersmalley
Copy link
Contributor

7.x/7.5: 88a4db0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants