Skip to content

chore: switch to prettier@2 defaults with 120 printWidth #1185

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

Merged
merged 5 commits into from
Jul 10, 2020

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented May 14, 2020

Issue #, if available:
Follow-up to #1183

Description of changes:
switch to prettier@2 defaults with 120 printWidth

Verified that vscode-prettier formats the page on edit

Screen recording

vscode-formatFileOnSave

prettier-vscode Output
["INFO" - 7:56:48 PM] Formatting /home/trivikr/workspace/aws-sdk-js-v3/clients/client-accessanalyzer/protocols/Aws_restJson1_1.ts
["INFO" - 7:56:48 PM] Using ignore file (if present) at /home/trivikr/workspace/aws-sdk-js-v3/.prettierignore
["INFO" - 7:56:48 PM] Loaded module '[email protected]' from '/home/trivikr/workspace/aws-sdk-js-v3/node_modules/prettier/index.js'
["INFO" - 7:56:48 PM] File Info:
{
  "ignored": false,
  "inferredParser": "typescript"
}
["INFO" - 7:56:48 PM] Detected local configuration (i.e. .prettierrc or .editorconfig), VS Code configuration will not be used
["INFO" - 7:56:48 PM] Prettier Options:
{
  "filepath": "/home/trivikr/workspace/aws-sdk-js-v3/clients/client-accessanalyzer/protocols/Aws_restJson1_1.ts",
  "parser": "typescript",
  "printWidth": 120
}
["INFO" - 7:56:49 PM] Formatting completed in 597.256223ms.

Verified that yarn generate-clients use printWidth 120 on CodeGen code

diff Screen Shot 2020-05-14 at 1 03 41 PM

Verified that husky formats files with printWidth 120 on precommit

terminal output
diff --git a/clients/client-accessanalyzer/protocols/Aws_restJson1_1.ts b/clients/client-accessanalyzer/protocols/Aws_restJson1_1.ts
index 683e228413..36ef40cf8d 100644
--- a/clients/client-accessanalyzer/protocols/Aws_restJson1_1.ts
+++ b/clients/client-accessanalyzer/protocols/Aws_restJson1_1.ts
@@ -1,4 +1,5 @@
-import { CreateAnalyzerCommandInput, CreateAnalyzerCommandOutput } from "../commands/CreateAnalyzerCommand";
+import { 
+  CreateAnalyzerCommandInput, CreateAnalyzerCommandOutput } from "../commands/CreateAnalyzerCommand";
 import { CreateArchiveRuleCommandInput, CreateArchiveRuleCommandOutput } from "../commands/CreateArchiveRuleCommand";
 import { DeleteAnalyzerCommandInput, DeleteAnalyzerCommandOutput } from "../commands/DeleteAnalyzerCommand";
 import { DeleteArchiveRuleCommandInput, DeleteArchiveRuleCommandOutput } from "../commands/DeleteArchiveRuleCommand";
% git add .

% git commit -m "test: test husky run on precommit"
husky > pre-commit (node v12.16.1)
  ✔ Preparing...
  ✔ Running tasks...
  ✖ Applying modifications...
    → Prevented an empty git commit!
  ✔ Reverting to original state because of errors...
  ✔ Cleaning up...

  ⚠ lint-staged prevented an empty git commit.
    Use the --allow-empty option to continue, or check your task configuration

Prevented an empty git commit!
husky > pre-commit hook failed (add --no-verify to bypass)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@trivikr trivikr marked this pull request as draft May 14, 2020 19:51
Copy link
Contributor

@alexforsyth alexforsyth left a comment

Choose a reason for hiding this comment

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

LGTM

@aws-sdk-js-automation

This comment has been minimized.

@aws-sdk-js-automation

This comment has been minimized.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@trivikr trivikr force-pushed the prettier2-defaults-with-120-printWidth branch from a663ebb to ebcf028 Compare July 10, 2020 17:48
@codecov-commenter

This comment has been minimized.

@trivikr
Copy link
Member Author

trivikr commented Jul 10, 2020

Screenshots

Utility packages

Before

before

After

after

Client packages

Before

before

After

after

@trivikr trivikr marked this pull request as ready for review July 10, 2020 18:03
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

👍 again

@trivikr
Copy link
Member Author

trivikr commented Jul 10, 2020

If anyone is visiting this PR in future.

Discussions related to printWidth:

[May 14, 2020, 12:08 PM] Kamat, Trivikram: I use 27-inch 4K monitor at home, and use Spectacle to use 66% of width for VSCode editor
I added editor.rulers for [80, 100, 120], and ruler for 120 doesn’t appear for default font-size (needs horizontal scrolling)
[May 14, 2020, 12:08 PM] Kamat, Trivikram: ☝️ that’s why I’m inclined to printWidth 100
But I’m open to change my mind if 120 printWidth is more helpful for you folks.
[May 14, 2020, 12:16 PM] Forsyth, Alex: 100 vs 120. 120 looks so much better
[May 14, 2020, 12:21 PM] Kamat, Trivikram: I’m open to both 100 and 120, but would prefer to vote for 100
Let’s finalize whichever Allan like, and stick with it.
[May 14, 2020, 12:23 PM] Kamat, Trivikram: These are all the prettier configurations https://prettier.io/docs/en/options.html
My recommendation is to remove prettier v1 default configs which we’d currently set https://github.com/aws/aws-sdk-js-v3/blob/117e9ca7b11b9dc0614a6af5eca90b44e7673ac2/.prettierrc#L3-L5
[May 14, 2020, 12:25 PM] Zheng, Allan: If Both Trivikram and me don’t have a stance, let use 120 width then
[May 14, 2020, 12:26 PM] Kamat, Trivikram: 120 it is! 🎉

Discussions related to decision:

[May 14, 2020, 12:30 PM] Kamat, Trivikram: If you don’t have a stance, I’ll remove the prettier@1 defaults. But will wait for your response.
[May 14, 2020, 12:32 PM] Forsyth, Alex: Personally dont care.
[May 14, 2020, 12:32 PM] Zheng, Allan: Yea if we decide to do a huge reformating this time, lets do every change we want
[May 14, 2020, 12:33 PM] Kamat, Trivikram: Cool! I’ll remove the prettier@v1 defaults
[May 14, 2020, 12:33 PM] Zheng, Allan: Similarly, we need to keep the config entries to make sure it won’t change when prettier prefers other format next time

@trivikr trivikr merged commit 97efeeb into aws:master Jul 10, 2020
@trivikr trivikr deleted the prettier2-defaults-with-120-printWidth branch July 10, 2020 18:14
trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Jul 15, 2020
trivikr added a commit that referenced this pull request Jul 16, 2020
trivikr added a commit that referenced this pull request Jul 16, 2020
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants