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

[chore]: Fixing Prettier use #1941

Merged
merged 5 commits into from
Oct 31, 2019
Merged

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Oct 29, 2019

Description

  1. Removes unnecessary prettier-check dependency and modifies pre-push to use prettier:check instead of prettier:validate.

  2. Creates a prettier.config.js file and uses the config version of --single-quote. I think this is a better experience for developers for future configuration on top of venia-concept.

Related Issue

Closes PWA-89.

Acceptance

Verification Stakeholders

@zetlen - I'd like to know if there are any additional changes necessary in venia-concept or in the scaffolding scripts.

Specification

Verification Steps

  1. Make some change in venia-concept that would fail prettier like adding double quotes to an import. Make sure you have "format on save" off so that it doesn't auto fix.
  2. Run yarn prettier:check from the root directory. It should complain.
  3. Go to packages/venia-concept and run yarn prettier. It should also complain.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@sirugh sirugh added version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. tooling Related to the developer tooling, including buildpack, webpack plugins, and debug UIs. labels Oct 29, 2019
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Oct 29, 2019

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-89.

Generated by 🚫 dangerJS against 39b4107

singleQuote: true
};

module.exports = config;
Copy link
Contributor

@jimbo jimbo Oct 30, 2019

Choose a reason for hiding this comment

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

Should this file be located in the pwa-studio root?

Edit: there's already one there. Does venia-concept not inherit it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to put context in this convo for later -- my intent was that venia-concept, when used as the root "theme" from the scaffolding script, would provide this config. I wasn't sure if the scaffolding script already did that work and copied the one in the root folder. If that is the case, then I can delete this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right move. Keeping these two files in sync shouldn't be a big problem, because they should VERY rarely change.

@sirugh
Copy link
Contributor Author

sirugh commented Oct 30, 2019 via email

@jimbo
Copy link
Contributor

jimbo commented Oct 30, 2019

I'll defer to @zetlen on this one. Looks fine to me.

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

I love it! On to QA.

singleQuote: true
};

module.exports = config;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right move. Keeping these two files in sync shouldn't be a big problem, because they should VERY rarely change.

@zetlen
Copy link
Contributor

zetlen commented Oct 30, 2019

Oh, @sirugh please open a dummy issue against this and link it so it passes Danger.

@dpatil-magento dpatil-magento merged commit a6df140 into develop Oct 31, 2019
@dpatil-magento dpatil-magento deleted the rugh/remove_prettier-check branch October 31, 2019 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-concept tooling Related to the developer tooling, including buildpack, webpack plugins, and debug UIs. version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants