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.

Sign up for GitHub

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

Jump to bottom

build: add prettier to format JSON, MD and YAML files #41880

Closed
wants to merge 2 commits into from
Closed

build: add prettier to format JSON, MD and YAML files #41880

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b631b2
build: add prettier to format JSON, MD and YAML files
alan-agius4 Apr 29, 2021
b0a92d9
fixup! build: add prettier to format JSON, MD and YAML files
alan-agius4 Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .ng-dev/format.ts
View file
Open in desktop
Expand Up @@ -23,5 +23,8 @@ export const format: FormatConfig = {
'!packages/compiler-cli/test/compliance/test_cases/**/*.js',
]
},
'prettier': {
'matchers': ['**/*.{json,yml,yaml,md}'],
},
'buildifier': true
};
8 changes: 8 additions & 0 deletions .prettierignore
View file
Open in desktop
@@ -0,0 +1,8 @@
/bazel-out/
gkalpak marked this conversation as resolved.
Show resolved Hide resolved
/dist
/goldens/public-api
/integration
third_party/
aio/content/examples/
aio/tools/transforms/templates/
alan-agius4 marked this conversation as resolved.
Show resolved Hide resolved
/CHANGELOG.md
Copy link
Member

Choose a reason for hiding this comment

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

We should ignore all CHANGELOG.md files as we have one in zone.js as well

alan-agius4 marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 6 additions & 0 deletions .prettierrc
View file
Open in desktop
@@ -0,0 +1,6 @@
{
"printWidth": 100,
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, currently we refrain from wrapping Markdown files at 100 chars (as it makes diffs quite verbose).
We try to keep one sentence per line, but I am not sure if prettier supports that 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @gkalpak here (as I think will @IgorMinar) - but LOL @jelbourn was just saying in a stand-up this week how he dislikes long lines in markdown. (Clearly does not have wrapping turned on in his IDE).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The printWidth setting doesn't apply to MarkDown files.

Copy link
Member

Choose a reason for hiding this comment

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

The printWidth setting doesn't apply to MarkDown files.

Are you sure? The prettier playground seems to indicate otherwise 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umm.. for some reason locally it didn’t have any effect when I tried it earlier. Will take another look later on.

That said, if this is applied to markdowns I believe there is no way to have different print widths for different file types. (Example: 100 for TS and JS and unlimited for MD).

Copy link
Member

Choose a reason for hiding this comment

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

I do feel pretty strongly about line breaking. I find having overflowing lines more difficult to read, edit, and review than breaking. Our internal markdown docs tool also enforces breaking at 100 columns and I've never found that difficult to review.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jelbourn Have you tried soft wrap? https://www.jetbrains.com/webstorm/guide/tips/soft-wraps/

GitHub automatically soft wraps as well. So if you configure your editor the file contents look great.

The biggest benefit of sentence per line formating is that if you add or remove a word in a sentence you don't trigger reformatting of the entire paragraph, instead all code review tools will highlight only the modified sentence with the added/removed word with extra emphasis.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just have a preference for not using soft wraps (since I don't like there being a disconnect between what I see and actual text-in-file).

Maybe I'm weird, but when a paragraph changes, I feel like you would generally re-read the surrounding text as well anyway to make sure the whole thing still makes sense. I don't see a reformatting as meaningfully changing the review burden in any way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be the best if the folks that do edit and review these docs the most - @aikidave and the tech writing crew - made this decision.

I think there is a lot of value in keeping the diffs minimal and clean, but Dave is the ultimate stakeholder here so we should adjust to his preferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to wait on @aikidave response before doing any changes.

"quoteProps": "preserve",
"singleQuote": true,
"trailingComma": "all"
Copy link
Contributor

Choose a reason for hiding this comment

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

another one to consider is "bracketSpacing": false: https://prettier.io/docs/en/options.html#bracket-spacing

}
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -194,6 +194,7 @@
"mutation-observer": "^1.0.3",
"nock": "^13.0.3",
"ora": "^5.0.0",
"prettier": "^2.2.1",
"rollup-plugin-hashbang": "^2.2.2",
"sauce-connect": "https://saucelabs.com/downloads/sc-4.6.2-linux.tar.gz",
"semver": "^7.3.5",
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Expand Up @@ -10655,6 +10655,11 @@ prepend-http@^2.0.0:
resolved "https://registry.yarnpkg.com/prepend-http/-/prepend-http-2.0.0.tgz#e92434bfa5ea8c19f41cdfd401d741a3c819d897"
integrity sha1-6SQ0v6XqjBn0HN/UAddBo8gZ2Jc=

prettier@^2.2.1:
version "2.2.1"
resolved "https://registry.yarnpkg.com/prettier/-/prettier-2.2.1.tgz#795a1a78dd52f073da0cd42b21f9c91381923ff5"
integrity sha512-PqyhM2yCjg/oKkFPtTGUojv7gnZAoG80ttl45O6x2Ug/rMJw4wcc9k6aaf2hibP7BGVCCM33gZoGjyvt9mm16Q==

pretty-bytes@^5.3.0:
version "5.6.0"
resolved "https://registry.yarnpkg.com/pretty-bytes/-/pretty-bytes-5.6.0.tgz#356256f643804773c82f64723fe78c92c62beaeb"
Expand Down