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

Increase prettier line width to 100 #20535

Merged
merged 5 commits into from
Jul 9, 2018
Merged

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Jul 6, 2018

In this PR I increase the max line width in prettier to 100. Since we enforce the prettier styling for TypeScript I wanted to put this up for discussion if 80 (which it was right now) is really reasonable. You can also check the auto fixes in this PR to get a feeling how 80 chars vs 100 chars will look like.

Please feel free to vote up/down on this PR to switch to 100 chars line width (and in case you also vote up, feel free to leave a review).

@timroes timroes added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jul 6, 2018
@markov00
Copy link
Member

markov00 commented Jul 6, 2018

Our style guide says 120 js_style_guide

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor Author

timroes commented Jul 7, 2018

@markov00 I would understand Try to limit your lines to 80 characters. If it feels right, you can go up to 120 characters. As a flexible range between 80 and 120 🤔

Since people seem to like and upvote the 100 chars width, I would also change the styleguide accordingly, to be a bite more clear and not giving us a range.

Since we also enforce the line width in the reverse way, i.e. you cannot split up a line if it would fit into that setting, I would rather limit it to the 100 (that's somewhere in the middle of 80 and 120), since I think 120 char lines could tend to be rather long as a "must" have.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor

I'm neither here nor there on 80 vs 100, could go either way, but

I would also change the styleguide accordingly, to be a bit more clear and not giving us a range.

I'm +1 on picking a width and removing the ambiguity.

@ppisljar
Copy link
Member

ppisljar commented Jul 9, 2018

i agree with @stacey-gammon i am also ok with updating only the docs

@timroes
Copy link
Contributor Author

timroes commented Jul 9, 2018

@stacey-gammon @ppisljar I already updated the docs, to be less ambiguous about the length, and clearly set it to 100 with this PR.

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

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

I'm not happy with how prettier tries to cram as much code on a line as possible, but I'm glad we're picking an explicit line length!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes merged commit cb5ee01 into elastic:master Jul 9, 2018
@timroes timroes deleted the ts-line-width branch July 9, 2018 20:50
timroes added a commit to timroes/kibana that referenced this pull request Jul 9, 2018
* Increase prettier line width to 100

* Fix packages JS prettier

* Change style guide to 100 width

* Fix line-width in latest master changes
timroes added a commit that referenced this pull request Jul 10, 2018
* Increase prettier line width to 100

* Fix packages JS prettier

* Change style guide to 100 width

* Fix line-width in latest master changes
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.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants