Closed
Conversation
mshustov
added
chore
Team:Core
Contributor
|
Pinging @elastic/kibana-platform (Team:Platform) |
mshustov
commented
Apr 16, 2020
| 'error', | ||
| { | ||
| endOfLine: 'auto', | ||
| arrowParens: 'avoid', |
Contributor
Author
There was a problem hiding this comment.
Should we keep the BWC with the current style?
Contributor
There was a problem hiding this comment.
I think we should avoid adding prettier configuration at this time, lets pull the bandaid off and move forward with prettier.
Contributor
Author
|
14577 errors. mostly because of non-configurable https://prettier.io/blog/2020/03/21/2.0.0.html#always-add-a-space-after-the-function-keyword-3903httpsgithubcomprettierprettierpull3903-by-j-f1httpsgithubcomj-f1-josephfrazierhttpsgithubcomjosephfrazier-sosukesuzukihttpsgithubcomsosukesuzuki-thorn0httpsgithubcomthorn0-7516httpsgithubcomprettierprettierpull7516-by-bakkothttpsgithubcombakkot @spalger what is the best way to land it? I suspect we can run the auto-fix, but it will cause tons of conflicts. |
Contributor
|
@restrry in the past we've mostly advised people to update prettier in their local repo, run We could instead write a script that does the prettier update and then just runs prettier on the files they've changed from master, commit the prettier changes, revert the changes to package.json and yarn.lock, and then merge master and I would expect it to work... |
Contributor
|
Additionally, we should do this later on a Friday to make sure that the fewest people possible have green CI without merging master. By merging this upgrade late on a Friday and sending an email we should minimize the chance someone will merge in changes which violate the updated eslint rules. |
Contributor
Author
The problem with this approach that it hides modifications, thus people could be surprised to find changes they didn't commit to fixing the errors they didn't see. Probably I can merge the current PR on the upcoming weekend and merge the master into all the PR branches that aren't outdated yet?
I believe they still need to merge master. How they could get the script in their branches, otherwise? Perhaps, we could merge the script a week before the current PR to increase the likelihood that all the pending PRs already have prettier v2 compatible code-style. I outlined the script to run prettier on changed files in a branch. https://github.com/elastic/kibana/compare/master...restrry:run-local-prettier?expand=1 |
Contributor
|
Alright, I really dig the script you've got there, lets get that merged right before we merge the prettier changes so that we can send, in an email, the following instructions along with the commits for the changes in 7.x and master:
Additionally, I think the script should be tweaked a little
|
Contributor
|
Oh, you already merged the prettier script :) In the email sent to people preparing for this I think we should still tell them to merge in the commit right before we merge this PR, so that they have all possible merge conflicts resolved before trying to merge this PR. |
Contributor
|
Also, lets update https://github.com/elastic/kibana/blob/master/test/scripts/jenkins_unit.sh#L4 temporarily to apply It's possible that doing a --fix will actually fail like it does locally, so we might want to try: |
Contributor
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/lens/smokescreen·ts.lens app lens smokescreen tests should allow seamless transition to and from table view and add a filterStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Required to support new syntax introduced in TypeScript v3.8
Draft PR to check how many errors we have. I interrupted
yarn lint:esafter 1h on my laptop 😄The full list of all the changes https://prettier.io/blog/2020/03/21/2.0.0.html