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

[github action] display black result in job summary #3688

Merged
merged 5 commits into from
May 15, 2023
Merged

Conversation

tieum
Copy link
Contributor

@tieum tieum commented May 13, 2023

Description

This PR updates the black github action to display the output of black in the job summary

Checklist

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Result looks like this:

- erroring with verbosity on

screenshot-2023-05-13-1727

- success with verbosity off

screenshot-2023-05-13-1741

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.

Just checking if we should care about the new lines, I feel it might mess up output people maybe parse and make things harder to read ...

action/main.py Outdated
@@ -32,7 +32,7 @@
describe_name = line[len("describe-name: ") :].rstrip()
break
if not describe_name:
print("::error::Failed to detect action version.", flush=True)
sys.stderr.write("::error::Failed to detect action version.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we care about the newline here you've removed? If so I recommend sticking with print cause I believe it will send correct newline in windows ...

Suggested change
sys.stderr.write("::error::Failed to detect action version.")
print("::error::Failed to detect action version.", file=sys.stderr, flush=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does need to be written to stderr so as errors do not appear in the job summary part (which won't parse it correctly) but the annotation one, like this:
screenshot-2023-05-13-1815
I've updated the PR with your feedback

action/main.py Outdated Show resolved Hide resolved
@tieum tieum requested a review from cooperlees May 14, 2023 12:35
@tieum
Copy link
Contributor Author

tieum commented May 14, 2023

I also checked action was correctly running on a Windows runner, an example run is here https://github.com/tieum/atuin-graph/actions/runs/4972553308

screenshot-2023-05-14-1507 screenshot-2023-05-14-1508

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.

Thanks for taking my suggestions into account. This all looks good to me now to get the nicer summary and does the right thing to stderr output for all platforms :)

@cooperlees cooperlees merged commit c97b9c5 into psf:main May 15, 2023
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.

2 participants