-
-
Notifications
You must be signed in to change notification settings - Fork 153
feat(action): add github action for cargo.toml formating #790
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks, great initiative! A few suggestions from a cursory read of the diff.
action.yml
Outdated
files: | ||
description: "Files or patterns to check" | ||
required: false | ||
default: "Cargo.toml" |
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.
I think it would be more universally useful to make the files required and don't specify a default.
action/entrypoint.sh
Outdated
fi | ||
log "jq: $(jq --version)" | ||
|
||
ARGS="format" |
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.
Would be great to also have support for taplo check
. And perhaps rename format
to fmt
(at least the docs seem to prefer fmt).
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.
as fmt
is an alias for format
, it's seems most logical to me to use format
unless fmt
presented as preferred way ?
I haven't explored the check
/lint
functionality, what it the interest in CI ? What you think it's should do ? How should it be integrated ?
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.
I cannot say anything about the preference of fmt
vs format
except that the docs seem to only mention fmt
The check would work similarly as for fmt
: run taplo check
, if it reports a problem fail. So my proposal would be to add two boolean toggles (both defaulting to true?), each for enabling/disabling the the fmt and check run.
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.
I have managed --check
option of format
command. There is a difference between format --check
and 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.
Commands:
lint Lint TOML documents [aliases: check, validate]
format Format TOML documents [aliases: fmt]
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.
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.
Ok, thanks.
Taplo is already usable in GHA via |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
action.yml
Outdated
INPUT_VERSION: ${{ inputs.version }} | ||
INPUT_FILES: ${{ inputs.files }} | ||
INPUT_WRITE_CHANGES: ${{ inputs.write_changes }} | ||
run: $GITHUB_ACTION_PATH/action/entrypoint.sh |
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.
Does it work for Windows?
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.
I haven't tested, but It's inspired from typos GHA, so it's should be.
Do you prefer a GitHub Action based on docker image, instead of release files ? I found no documentation about how to use taplo in GitHub Workflow. So a dedicated GitHub Action might be helpful to users. |
Docker-based actions are not available for macos and windows (based on https://docs.github.com/en/actions/sharing-automations/creating-actions/about-custom-actions#types-of-actions) |
513d148
to
0c8993f
Compare
If it's going to be action then it should just run - uses: taplo
with:
format: true/false
lint: true/false |
Although I think I'd prefer it to be in NodeJS and not shell |
I'm ok with this, I can update action in this direction.
I am not against it, but I haven't the knowledge to do this. So unless it's really simple and has examples to guide me, someone else will have to do it. |
@panekj : I have updated with simpler version. |
Note: this is a first version for comments
It's great to have Cargo.toml (by example) formatted on save in VSCode. But If it's not checked in CI, this is dependent of developer use
Even Better TOML
and process.TODO:
indent_string=" "
for Cargo.toml (likeEven Better TOML
)#470
Work inspired from https://github.com/marketplace/actions/typos-action