Skip to content
This repository has been archived by the owner on Oct 9, 2024. It is now read-only.

support ruff mode argument #19

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

yosmoc
Copy link
Contributor

@yosmoc yosmoc commented Mar 12, 2024

Related: #11
This PR allows Users to specify the mode of ruff to be 'check' or 'format' using the mode input.

Users can now specify the mode of ruff to be 'check' or 'format' using the mode input.
@yosmoc yosmoc force-pushed the configure_check_or_format_mode branch from aede224 to 17a2e52 Compare March 12, 2024 23:07
@yosmoc yosmoc marked this pull request as ready for review March 12, 2024 23:14
@brucearctor
Copy link
Contributor

@yosmoc -- thanks! Looking at this now.

A question: mode means lots of things in different contexts. Is there an alternate, more narrowly descriptive word that would be possible to use? Mode might be acceptable, but wanting to spend a moment to consider alternatives. Please advise thoughts.

@yosmoc
Copy link
Contributor Author

yosmoc commented Mar 18, 2024

@yosmoc -- thanks! Looking at this now.

A question: mode means lots of things in different contexts. Is there an alternate, more narrowly descriptive word that would be possible to use? Mode might be acceptable, but wanting to spend a moment to consider alternatives. Please advise thoughts.

Thanks. How about these alternatives?

  • check_or_format: check or format
  • linter_or_formatter: linter or formatter
  • subcommand: check or format

README.md Outdated
- src: default, '.'

```yaml
- uses: chartboost/ruff-action@v1
with:
src: "./src"
mode: "check"
Copy link
Contributor

Choose a reason for hiding this comment

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

if the default is check, why would we include that in the example? Wouldn't users keep it blank since to check it wouldn't need to be specified?

@brucearctor
Copy link
Contributor

@yosmoc -- looking a bit closer: is there a reason the existing args doesn't suffice? I think you could place 'check' in that if desiring to check? If that indeed works, maybe we do not need an additional parameter, and could just update the docs?

@yosmoc
Copy link
Contributor Author

yosmoc commented Mar 19, 2024

@yosmoc -- looking a bit closer: is there a reason the existing args doesn't suffice? I think you could place 'check' in that if desiring to check? If that indeed works, maybe we do not need an additional parameter, and could just update the docs?

@brucearctor Sure! If we provide an example of ruff format in the documentation, that should be sufficient. A user normally runs ruff format with the --check option in ci, I believed it would be beneficial if the action automatically adds the --check option for the first time. (You also mentioned in here. #11 (comment) )

@yosmoc
Copy link
Contributor Author

yosmoc commented Mar 19, 2024

@brucearctor I reverted my original commit and updated the README file. Would you please check it again?

```yaml
- uses: chartboost/ruff-action@v1
with:
args: 'format --check'
Copy link
Contributor

@brucearctor brucearctor Mar 19, 2024

Choose a reason for hiding this comment

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

https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules

Screenshot 2024-03-19 at 12 03 30 PM

Are you sure people would want to use format --check [ see docs from link/screenshot ] ... I imagine generally want to use either format or check [ or even format followed by check ]? Otherwise -- and I haven't tested -- is the failure message sufficiently easy to debug?

I'm fine with this, if you think correct, but please think through. If wanting format --check, maybe we link to the doc or make the failure code clear to help people with potential debugging? Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ... maybe I missed the development. ruff formatter is different than linter! So, ruff format --check, is checking that the file is formatted. In this case, formatter is roughly a replacement for black? If that's correct, then indeed this makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe was confusing 'format' with 'fix' option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Ruff formatter is replacement for black. The Ruff Formatter - Ruff

Copy link
Contributor

@brucearctor brucearctor Mar 21, 2024

Choose a reason for hiding this comment

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

would you put a little not in the docs as part of your PR about that? I imagine - since I got confused others might as well? Otherwise, we're great to go.

@brucearctor brucearctor merged commit 47bc082 into ChartBoost:main Mar 25, 2024
1 check passed
@yosmoc yosmoc deleted the configure_check_or_format_mode branch March 26, 2024 05:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants