Skip to content

Conversation

gwen-lg
Copy link

@gwen-lg gwen-lg commented May 17, 2025

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:

  • manage config with indent_string=" " for Cargo.toml (like Even Better TOML)
  • convert output to display beautiful error/warning in github
  • feature for allow automatic patch commits ?

#470

Work inspired from https://github.com/marketplace/actions/typos-action

Copy link

@tobiasdiez tobiasdiez left a 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"

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.

fi
log "jq: $(jq --version)"

ARGS="format"

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).

Copy link
Author

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 ?

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.

Copy link
Author

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 ?

Copy link
Collaborator

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]

Choose a reason for hiding this comment

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

Copy link
Author

@gwen-lg gwen-lg May 26, 2025

Choose a reason for hiding this comment

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

Ok, thanks.

@panekj
Copy link
Collaborator

panekj commented May 25, 2025

Taplo is already usable in GHA via uses: docker://tamasfe/taplo

@JounQin

This comment was marked as off-topic.

@panekj

This comment was marked as off-topic.

@JounQin

This comment was marked as off-topic.

@panekj

This comment was marked as off-topic.

@JounQin

This comment was marked as off-topic.

@panekj

This comment was marked as off-topic.

@JounQin

This comment was marked as off-topic.

@panekj

This comment was marked as off-topic.

@JounQin

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
Copy link
Collaborator

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?

Copy link
Author

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.

@gwen-lg
Copy link
Author

gwen-lg commented May 26, 2025

Taplo is already usable in GHA via uses: docker://tamasfe/taplo

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.

@tobiasdiez
Copy link

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)

@gwen-lg gwen-lg force-pushed the github_action branch 2 times, most recently from 513d148 to 0c8993f Compare May 26, 2025 13:36
@panekj
Copy link
Collaborator

panekj commented May 26, 2025

If it's going to be action then it should just run taplo, adding commands checking and file arguments is super awkward when the configuration is supposed to live in /taplo.toml (or /.taplo.toml)

- uses: taplo
  with:
    format: true/false
    lint: true/false

@panekj
Copy link
Collaborator

panekj commented May 26, 2025

Although I think I'd prefer it to be in NodeJS and not shell

@gwen-lg
Copy link
Author

gwen-lg commented May 26, 2025

If it's going to be action then it should just run taplo, adding commands checking and file arguments is super awkward when the configuration is supposed to live in /taplo.toml (or /.taplo.toml)

- uses: taplo
  with:
    format: true/false
    lint: true/false

I'm ok with this, I can update action in this direction.

Although I think I'd prefer it to be in NodeJS and not shell

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.

@gwen-lg
Copy link
Author

gwen-lg commented May 26, 2025

@panekj : I have updated with simpler version.
I have keep the setup of version, and format_write_changes this second can also be removed if it's preferred.
I have add a small part for doc in README.md, I haven't take time to create a page in website documentation, perhaps it's better/or complementary to README.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants