-
Notifications
You must be signed in to change notification settings - Fork 97
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
Adding prettier command and running it for the first time #7893
Conversation
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Lets have a discussion before adding new required tools. For starters, whats the benefit of adding prettier to our contribution process? Why is this worth having every contributor install it and learn how to use it? |
15cff8e
to
fd548b4
Compare
fd548b4
to
9ea5db0
Compare
9ea5db0
to
fdb5c75
Compare
fdb5c75
to
960414d
Compare
960414d
to
ee342b6
Compare
ee342b6
to
9c712f0
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
build/prettier.mk
Outdated
|
||
PRETTIER_VERSION := 3.3.3 | ||
|
||
prettier-check: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documentation for these, use the other files as an example. When it's done you should see the new targets in the command line help printed by make
.
build/prettier.mk
Outdated
|
||
prettier-format: | ||
@npx prettier@$(PRETTIER_VERSION) --write "*/**/*.{ts,js,mjs,json}" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Should we name/group these commands based on their purpose rather than their tool?
Example:
make format-check
VS
make prettier-check
Consider what would happen if we add more formatting tools in the future.
@npx prettier@$(PRETTIER_VERSION) --check "*/**/*.{ts,js,mjs,json}" | ||
|
||
prettier-format: | ||
@npx prettier@$(PRETTIER_VERSION) --write "*/**/*.{ts,js,mjs,json}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the experience like if node is not installed? What about when it's an ancient version of node?
|
||
```sh | ||
make me prettier | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be less clever about the name?
I think this guidance should be combined with line 14. The idea is that we're giving contributors simple advice. Saying "run make format build test lint
" is simpler than saying "run make build test lint
and then run make prettier
".
9c712f0
to
76aa6ba
Compare
76aa6ba
to
f68faac
Compare
f68faac
to
dc1c085
Compare
dc1c085
to
163473a
Compare
163473a
to
ebcca56
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7893 +/- ##
=======================================
Coverage 59.33% 59.34%
=======================================
Files 560 560
Lines 37488 37487 -1
=======================================
+ Hits 22245 22247 +2
+ Misses 13697 13695 -2
+ Partials 1546 1545 -1 ☔ View full report in Codecov by Sentry. |
ebcca56
to
a27f697
Compare
Also running the prettier command for the first time Signed-off-by: ytimocin <[email protected]>
a27f697
to
713dee3
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
if: steps.format-check.outcome == 'failure' | ||
run: | | ||
echo "Format check failed. Please fix the formatting issues. You can run 'make format-write' to update the code." | ||
exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
Description
Adding prettier command and running it for the first time.
Type of change
Fixes: #issue_number