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

Add Prettier shared config package #20026

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Add Prettier shared config package #20026

merged 4 commits into from
Mar 13, 2020

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Feb 4, 2020 •
edited by gziolo
Loading

Description

Add Prettier shared config package

How has this been tested?

npm run test-unit

npm run test-lint-js

npm run lint-format-js

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ntwb ntwb added the [Tool] Prettier config /packages/prettier-config label Feb 4, 2020
@ntwb ntwb requested a review from gziolo February 4, 2020 09:39
@ntwb ntwb self-assigned this Feb 4, 2020
@ntwb ntwb added the npm Packages Related to npm packages label Feb 4, 2020
gziolo
gziolo reviewed Feb 4, 2020
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

We have configs for other tools so it might make sense to keep one for Prettier. Does it have any custom rule for WordPress setup enforced with the Prettier fork? We could add a note that it can be used with @wordpress/scripts to avoid the hassle and maybe mention the fork as well?

packages/prettier-config/README.md Outdated Show resolved Hide resolved
packages/prettier-config/README.md Outdated Show resolved Hide resolved
@gziolo gziolo requested a review from jsnajdr February 4, 2020 09:46
@gziolo
Copy link
Member

gziolo commented Feb 4, 2020

I think npm run docs:build is necessary to update the manifest for WordPress developers docs. It should make Travis pass.

Can we also update the existing config in @wordpress/scripts to use this new package? In addition, we probably should list it as a dev dependency in the root package.json file (we do it for other configs) and as production dependency in @wordpress/scripts.

This was referenced Mar 2, 2020
Closed
Closed
ntwb and others added 3 commits March 13, 2020 09:40
@gziolo gziolo force-pushed the add/prettier-config-package branch from f3b111e to cc873ff Compare March 13, 2020 08:47
@github-actions
Copy link

github-actions bot commented Mar 13, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 856 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 111 kB 0 B
build/block-library/style-rtl.css 7.38 kB 0 B
build/block-library/style.css 7.39 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.21 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.45 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 779 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@gziolo gziolo force-pushed the add/prettier-config-package branch from cc873ff to 2f33319 Compare March 13, 2020 08:53
@gziolo gziolo force-pushed the add/prettier-config-package branch from 2f33319 to 7539155 Compare March 13, 2020 08:56
gziolo
gziolo approved these changes Mar 13, 2020
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

@ntwb – I rebased this branch with master and addressed my own feedback. Feel free to edit my commit if you disagree with any of the changes applied.

This PR should be ready to go.

@ntwb
Copy link
Member Author

ntwb commented Mar 13, 2020

Thanks, sorry, have been on holidays and catching up on all the things.

Looks good to me 👍🏼

ntwb
ntwb commented Mar 13, 2020
View reviewed changes
Copy link
Member Author

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Can't approve my own PR 😬

@gziolo gziolo merged commit 56a12a6 into master Mar 13, 2020
@gziolo gziolo deleted the add/prettier-config-package branch March 13, 2020 13:22
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 13, 2020
@gziolo
Copy link
Member

gziolo commented Mar 13, 2020

Thanks, sorry, have been on holidays and catching up on all the things.

I hope you had great time 😃

aduth
aduth reviewed Mar 16, 2020
View reviewed changes
package-lock.json
Comment on lines -19773 to +19778
"resolved": "https://registry.npmjs.org/node-pre-gyp/-/node-pre-gyp-0.12.0.tgz",
"resolved": false,
Copy link
Member

Choose a reason for hiding this comment

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

These values keep being flipped back and forth. I summarized some findings at #16157 (comment) . Out of curiosity, do you use Linux for development (or another OS which would "opt out" of the optional dependencies here)? Or an older version of NPM (older than latest 6.14.2)?

Copy link
Member

Choose a reason for hiding this comment

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

I'm now on npm 6.14.2, I believe I was on 6.14.1 at the time of this commit. I'm using macOS all the time. I have no idea why this value changes so often.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly suspect it's related to the fact that the dependencies are "optional", though it seems like in macOS they would not typically be skipped. In any case, it does feel like it's something largely on npm's end (i.e. I do not think it's correct that this would ever be false). Wondering if we could do something to address it in the meantime though. For example, we could create a script which is run during the build and verifies deeply that there are no "resolved": false in package-lock.json. But I'm not sure yet how we make these issues actionable from the perspective of the pull request author; in other words, how it would be expected that you would fix the problem.

@aduth aduth mentioned this pull request Mar 19, 2020
Closed
@gregrickaby gregrickaby mentioned this pull request Mar 27, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@gziolo gziolo gziolo approved these changes

@jsnajdr jsnajdr Awaiting requested review from jsnajdr

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@nerrad nerrad Awaiting requested review from nerrad

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@ntwb ntwb

Labels
npm Packages Related to npm packages [Tool] Prettier config /packages/prettier-config
Projects
None yet
Milestone
Gutenberg 7.8
Development

Successfully merging this pull request may close these issues.
3 participants
@ntwb @gziolo @aduth