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

Release gh action #1909

Merged
merged 4 commits into from
Jan 8, 2021
Merged

Release gh action #1909

merged 4 commits into from
Jan 8, 2021

Conversation

rickstaa
Copy link
Contributor

@rickstaa rickstaa commented Jan 6, 2021

Makes some changes to the gh-action such that it can be released to the marketplace. I could be wrong, but to my knowledge users do not have access to the args docker argument when using the action. I, therefore, added a black_args input argument.

This commit fixes a bug which caused not all input arguments were forwarder to the black formatter.
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Please fix the lint - Rest looks good.

Fuzz: libcst errors parsing it seems - I don't understand what but it can't be caused by this PR:

 File "<string>", line 6, in __init__
  • So I'll ignore that and merge and look to fix elsewhere.

README.md Show resolved Hide resolved
@ichard26
Copy link
Collaborator

ichard26 commented Jan 8, 2021

Fuzz: libcst errors parsing it seems - I don't understand what but it can't be caused by this PR:

This PR is hitting #1012 by chance. Also, LibCST? You mean the obviously better blib2to3 :)

edit: the LibCST vs blib2to3 thing was a joke, Black is gonna need to retire it soon because the parser change in CPython

Co-authored-by: Cooper Lees <me@cooperlees.com>
@cooperlees
Copy link
Collaborator

Ignoring fuzzer changes we need to look into elsewhere and merging this to unblock the release to market place

@ichard26
Copy link
Collaborator

but to my knowledge users do not have access to the args docker argument when using the action

Maybe it would be worth investigating https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runsargs because IMO this is pretty bad UX even it works. Thanks for working on this though!

@rickstaa
Copy link
Contributor Author

@ichard26 Thanks for your comment! Your correct, I also noticed this some weeks ago on another action. I didn't have the time yet to open a new pull request. I think the code can be simplified to:

#!/bin/sh
set -e

black_args=(".")
if [ "${INPUT_BLACK_ARGS}" != "" ]; then
  black_args+=(${INPUT_BLACK_ARGS})
else
  # Default (if no args provided).
  black_args+=("--check" "--diff")
fi

sh -c "black . ${black_args[*]}"

@ichard26, @cooperlees Let me know what you think.

@ghost
Copy link

ghost commented Apr 16, 2021

This is a kind reminder. @ichard26 @cooperlees

Thank you a lot, @rickstaa! I use this action and find it quite useful.

By the way, that script may give . to black twice. Is it intended?

Also, now that we only have $INPUT_BLACK_ARGS which is just a string (rather than an array of args), the script may be able to be further simplified like this.

#!/bin/sh -e

args=$INPUT_BLACK_ARGS
if [ -z "$args" ]; then
  # Default (If no args provided).
  args='. --check --diff'
fi

sh -c "black $args"

Could you make a PR, @rickstaa? Or I'll handle it if you are busy.


EDIT: Or, we can just set the default value . --check --diff in action.yml.

default: ""

Then the script would be even simpler.

#!/bin/sh -e

sh -c "black $INPUT_BLACK_ARGS"

@rickstaa
Copy link
Contributor Author

@simaki Thanks a lot for your comment! I this pull request, I did my best also to support the docker args option. But as pointed out by @ichard26, this option is only needed during debugger and not useful for the user. I think your last solution is the best one:

#!/bin/sh -e

sh -c "black $INPUT_BLACK_ARGS"

If @cooperlees agrees, I think it would be great if you could create a pull request.

@cooperlees
Copy link
Collaborator

I see no problem with any of this.

ichard26 pushed a commit that referenced this pull request May 6, 2021
This commit simplifies entrypoint.sh for GitHub Actions by removing
duplication of args and black_args (cf. #1909).

The reason why #1909 uses the input id black_args is to avoid an overlap
with args, but this naming seems redundant. So let me suggest option
and src, which are consistent with CLI. Backward compatibility is
guaranteed; Users can still use black_args as well.

Commit history pre-merge:
* Simplify GitHub Action entrypoint (#1909)
* Fix prettier
* Emit a warning message when `black_args` is used

  This deprecation should be visible in GitHub Action's UI now.

  Co-authored-by: Shota Ray Imaki <shota.imaki.0801@gmail.com>
  Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>
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