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

Improve the precommit hook #11107

Merged
merged 4 commits into from Sep 10, 2020
Merged

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Sep 8, 2020

In this PR:

Epic: #11105.

**/review/*.api.md
**/*.yml
**/*.yaml
**/*.d.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

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 also curious if this makes a difference

Copy link
Member Author

Choose a reason for hiding this comment

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

pretty-quick applies the rules in .prettierignore file in a way different from prettier. The former applies the rules relative to the root of source control tree (see here) but the latter applies the rules relative to the ignore path (see here). It is confusing and there is an issue opened for it here: prettier/pretty-quick#95 (comment).

"autoinstallerName": "rush-prettier",

// This will invoke common/autoinstallers/rush-prettier/node_modules/.bin/pretty-quick
"shellCommand": "pretty-quick --staged --no-restage"
Copy link
Contributor

Choose a reason for hiding this comment

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

looks cool!

Copy link
Contributor

@sadasant sadasant 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! Please wait for another approval before merging.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for researching the deep Rush magic for us Deya! 👍

@deyaaeldeen deyaaeldeen added the Prettier config Prettier setup and configurations label Sep 9, 2020
@deyaaeldeen
Copy link
Member Author

@ramya-rao-a I rebased your branch in this issue #10770 on this one and found the formatting changes applied to be reasonable:
Capture1

@xirzec
Copy link
Member

xirzec commented Sep 10, 2020

@ramya-rao-a can you confirm if this change is good for you? I'd love to get it into master soon.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Let's go!

@deyaaeldeen deyaaeldeen merged commit e1b81ab into Azure:master Sep 10, 2020
@deyaaeldeen deyaaeldeen deleted the improve-precommit-hook branch September 10, 2020 12:32
This was referenced Sep 10, 2020
sadasant pushed a commit to sadasant/azure-sdk-for-js that referenced this pull request Sep 10, 2020
* Create rush-prettier autoinstaller

* Update the hook script to use rush command

* do not restage formatted files

* update .prettierignore to work with pretty-quick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prettier config Prettier setup and configurations
Projects
None yet
5 participants