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

feat: add args input #13

Merged
merged 3 commits into from
Sep 24, 2024
Merged

feat: add args input #13

merged 3 commits into from
Sep 24, 2024

Conversation

znd4
Copy link
Contributor

@znd4 znd4 commented Sep 22, 2024

Adds args input to allow for passing arbitrary additional arguments to dprint check

Alternatives considered

Add specific inputs for the things that I need:

  • files
  • --excludes
  • --allow-no-files

Adds `args` input to allow for passing arbitrary additional arguments to
`dprint check`

**Alternatives**

Add specific inputs for the things that I need:

- `files`
- `--excludes`
@CLAassistant
Copy link

CLAassistant commented Sep 22, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM Thank you!

Could you document this in the README briefly?

@znd4
Copy link
Contributor Author

znd4 commented Sep 24, 2024

thanks for the quick review! Does the example I added work? (It's the actual usecase I need it for)

Copy link
Collaborator

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Yep that should work, just one clarifying syntax question :)

Comment on lines +65 to +67
args: >-
--allow-no-files
${{ steps.changed-files.outputs.all_changed_files }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
args: >-
--allow-no-files
${{ steps.changed-files.outputs.all_changed_files }}
args: |
--allow-no-files
${{ steps.changed-files.outputs.all_changed_files }}

I am not familiar with that yaml syntax, does a multi-line string also work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

> makes it so that lines are separated with spaces instead of newlines, such that

a: >
  b
  c

Becomes

{"a": "b c\n"}

- chomps the trailing newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use https://yaml-multiline.info/ ~ once per month for silly yaml features like this

Copy link
Contributor Author

@znd4 znd4 Sep 24, 2024

Choose a reason for hiding this comment

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

I think the problem with | here is that it would end up with the command

dprint check --allow-no-files
file1 file2 ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I use yaml-multiline.info ~ once per month for silly yaml features like this

It is cool and scary at the same time that yaml supports this. Thanks for explaining!

Comment on lines 44 to 46



Copy link
Collaborator

Choose a reason for hiding this comment

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

What is with all this whitespace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@znd4 znd4 requested a review from thomaseizinger September 24, 2024 22:52
@thomaseizinger thomaseizinger changed the title ci: Add args feat: add args input Sep 24, 2024
@thomaseizinger thomaseizinger merged commit 636e2a0 into dprint:main Sep 24, 2024
4 checks passed
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.

3 participants