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

remove pragma options from .prettierrc #29403

Merged
merged 4 commits into from
Dec 14, 2018
Merged

remove pragma options from .prettierrc #29403

merged 4 commits into from
Dec 14, 2018

Conversation

flootr
Copy link
Contributor

@flootr flootr commented Dec 13, 2018

Changes proposed in this Pull Request

Nowadays we always use Prettier regardless of the pragma options. It is that from SASS files this pragma is not removed and ends up in production css files - which is not what we want. So here's an attempt to just remove it entirely.

  • Remove pragma related options from .prettierrc

Testing instructions

  • Does [your-favorite-editor] still format files regardless of if there's a pragma at the top of the file?

@flootr flootr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build labels Dec 13, 2018
@flootr flootr self-assigned this Dec 13, 2018
@flootr flootr requested a review from a team December 13, 2018 12:12
@matticbot
Copy link
Contributor

@jsnajdr
Copy link
Member

jsnajdr commented Dec 13, 2018

There are Prettier invocations with --require-pragma in the pre-commit hook and in npm run reformat-files. That means that Prettier reformats only files with the pragma present when run from these scripts.

We should remove that option from there, too.

@flootr
Copy link
Contributor Author

flootr commented Dec 13, 2018

We should remove that option from there, too.

done in f925955 👍

@blowery
Copy link
Contributor

blowery commented Dec 13, 2018

from #29404 (comment) we just need to add a .prettierignore to ignore any files that we currently have that do not have the pragma already.

@blowery
Copy link
Contributor

blowery commented Dec 13, 2018

candidates: git grep -L "@format" | grep -E "\.js(x)?$"

@blowery
Copy link
Contributor

blowery commented Dec 13, 2018

bin/build-docker.js
bin/eslint-branch.js
bin/install-if-no-packages.js
bin/loader-stats.js
bin/pre-push-hook.js
bin/process-extensions-scss.js
bin/stacktrace-translate.js
bin/welcome.js
client/components/formatted-date/to-current-locale.js
client/extensions/woocommerce/woocommerce-services/state/shipping-label/constants.js
client/extensions/woocommerce/woocommerce-services/views/shipping-label/label-purchase-modal/customs-step/index.js
client/extensions/woocommerce/woocommerce-services/views/shipping-label/label-purchase-modal/customs-step/item-row-header.js
client/extensions/woocommerce/woocommerce-services/views/shipping-label/label-purchase-modal/customs-step/item-row.js
client/extensions/woocommerce/woocommerce-services/views/shipping-label/label-purchase-modal/customs-step/package-row.js
client/gutenberg-blocks/index.jsx
client/gutenberg/editor/edit-post/components/header/plugins-more-menu-group/index.js
client/notifications/src/panel/Notifications.jsx
client/notifications/src/panel/comment-replies-cache/index.js
client/notifications/src/panel/flux/app-actions.js
client/notifications/src/panel/flux/constants.js
client/notifications/src/panel/flux/input-actions.js
client/notifications/src/panel/flux/input-reducers.js
client/notifications/src/panel/flux/note-actions.js
client/notifications/src/panel/flux/store.js
client/notifications/src/panel/helpers/input.js
client/notifications/src/panel/helpers/notes.js
client/notifications/src/panel/helpers/stats.js
client/notifications/src/panel/indices-to-html/index.js
client/notifications/src/panel/rest-client/bump-stat.js
client/notifications/src/panel/rest-client/index.js
client/notifications/src/panel/rest-client/wpcom.js
client/notifications/src/panel/state/action-middleware/index.js
client/notifications/src/panel/state/action-middleware/notes/index.js
client/notifications/src/panel/state/action-middleware/notes/read-status.js
client/notifications/src/panel/state/action-middleware/overrides/index.js
client/notifications/src/panel/state/action-middleware/suggestions/index.js
client/notifications/src/panel/state/action-middleware/ui/index.js
client/notifications/src/panel/state/action-middleware/utils.js
client/notifications/src/panel/state/action-types.js
client/notifications/src/panel/state/actions.js
client/notifications/src/panel/state/index.js
client/notifications/src/panel/state/notes/actions.js
client/notifications/src/panel/state/notes/reducer.js
client/notifications/src/panel/state/selectors/get-all-notes.js
client/notifications/src/panel/state/selectors/get-filter-name.js
client/notifications/src/panel/state/selectors/get-is-loading.js
client/notifications/src/panel/state/selectors/get-is-note-approved.js
client/notifications/src/panel/state/selectors/get-is-note-hidden.js
client/notifications/src/panel/state/selectors/get-is-note-liked.js
client/notifications/src/panel/state/selectors/get-is-note-read.js
client/notifications/src/panel/state/selectors/get-is-panel-open.js
client/notifications/src/panel/state/selectors/get-note.js
client/notifications/src/panel/state/selectors/get-notes.js
client/notifications/src/panel/state/selectors/get-selected-note-id.js
client/notifications/src/panel/state/selectors/get-site-suggestions.js
client/notifications/src/panel/state/selectors/get-suggestions.js
client/notifications/src/panel/state/selectors/get-ui.js
client/notifications/src/panel/state/selectors/has-site-suggestions.js
client/notifications/src/panel/state/selectors/note-has-filtered-read.js
client/notifications/src/panel/state/suggestions/actions.js
client/notifications/src/panel/state/suggestions/reducer.js
client/notifications/src/panel/state/ui/actions.js
client/notifications/src/panel/state/ui/reducer.js
client/notifications/src/panel/suggestions/index.jsx
client/notifications/src/panel/suggestions/suggestion.jsx
client/notifications/src/panel/templates/action-button.jsx
client/notifications/src/panel/templates/actions.jsx
client/notifications/src/panel/templates/block-comment.jsx
client/notifications/src/panel/templates/block-post.jsx
client/notifications/src/panel/templates/block-user.jsx
client/notifications/src/panel/templates/body.jsx
client/notifications/src/panel/templates/button-approve.jsx
client/notifications/src/panel/templates/button-back.jsx
client/notifications/src/panel/templates/button-edit.jsx
client/notifications/src/panel/templates/button-like.jsx
client/notifications/src/panel/templates/button-spam.jsx
client/notifications/src/panel/templates/button-trash.jsx
client/notifications/src/panel/templates/comment-reply-input.jsx
client/notifications/src/panel/templates/container-hotkey.jsx
client/notifications/src/panel/templates/empty-message.jsx
client/notifications/src/panel/templates/error.jsx
client/notifications/src/panel/templates/filter-bar-controller.js
client/notifications/src/panel/templates/filter-bar.jsx
client/notifications/src/panel/templates/filters.js
client/notifications/src/panel/templates/follow-link.jsx
client/notifications/src/panel/templates/functions.jsx
client/notifications/src/panel/templates/gridicons.jsx
client/notifications/src/panel/templates/image-loader.jsx
client/notifications/src/panel/templates/index.jsx
client/notifications/src/panel/templates/list-header.jsx
client/notifications/src/panel/templates/nav-button.jsx
client/notifications/src/panel/templates/note-list.jsx
client/notifications/src/panel/templates/note.jsx
client/notifications/src/panel/templates/preface.jsx
client/notifications/src/panel/templates/status-bar.jsx
client/notifications/src/panel/templates/summary-in-list.jsx
client/notifications/src/panel/templates/summary-in-single.jsx
client/notifications/src/panel/templates/undo-list-item.jsx
client/notifications/src/panel/utils/link-interceptor.js
client/notifications/src/panel/utils/noticon2gridicon.js
client/notifications/src/panel/utils/parse-json.js
client/notifications/src/standalone/auth-wrapper.jsx
client/notifications/src/standalone/index.js
client/notifications/src/standalone/messaging.js
client/notifications/src/standalone/offline-notes.js
client/notifications/src/standalone/offline-wrapper.jsx
client/state/data-layer/wpcom/me/transactions/supported-countries/test/index.js
client/state/selectors/get-notification-unseen-count.js
index.js
packages/i18n-calypso/_test/cli/examples/i18n-test-example-second-file.jsx
packages/i18n-calypso/_test/cli/examples/i18n-test-examples.jsx
packages/i18n-calypso/_test/cli/index.js
packages/i18n-calypso/_test/data/index.js
packages/i18n-calypso/_test/index.js
packages/i18n-calypso/_test/localize.js
packages/i18n-calypso/bin/index.js
packages/i18n-calypso/cli/extras/date.js
packages/i18n-calypso/cli/formatters/index.js
packages/i18n-calypso/cli/formatters/php.js
packages/i18n-calypso/cli/formatters/pot.js
packages/i18n-calypso/cli/index.js
packages/i18n-calypso/cli/preprocess-xgettextjs-match.js
packages/i18n-calypso/cli/util.js
packages/i18n-calypso/index.js
packages/i18n-calypso/lib/index.js
packages/i18n-calypso/lib/localize/index.js
packages/i18n-calypso/lib/number-format.js
server/devdocs/bin/generate-components-usage-stats.js

@blowery
Copy link
Contributor

blowery commented Dec 13, 2018

Seems like most of those should have the pragma and it was missed. Notifications and the i18n package are the current standouts.

@blowery
Copy link
Contributor

blowery commented Dec 14, 2018

in summary, i think we can forego adding a prettier ignore and just remove the pragma requirement entirely.

Copy link
Member

@jsnajdr jsnajdr 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! I tried to run npm run reformat-files and everything seems to be formatted reasonably without any failures. We can do the actual reformatting in separate PRs.

I plan to upgrade us to Prettier 1.15.3 before the end of the year. We're still on 1.14.0 from July 2018. But the Prettier development has cooled down a lot this year, so there won't be too many changes.

@jsnajdr
Copy link
Member

jsnajdr commented Dec 14, 2018

in summary, i think we can forego adding a prettier ignore and just remove the pragma requirement entirely.

I agree. We used to have problems with the scripts in bin/ where Prettier didn't understand the hashbang comment, but that's been fixed a while ago.

* Remove prettier pragma from sass files in /assets

* Remove prettier pragma from sass files in /client
@flootr flootr merged commit a6d699c into master Dec 14, 2018
@flootr flootr deleted the remove/prettier-pragma branch December 14, 2018 09:29
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 14, 2018
@sirreal sirreal mentioned this pull request Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants