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

Rewrite action with github-script #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brandonchinn178
Copy link
Member

@brandonchinn178 brandonchinn178 commented Apr 5, 2023

Javascript is much better than bash, with proper error mechanisms, better variables, better for-loops, etc. This also lets us use all the utilities normal GitHub actions can use, via actions/github-script, taking advantage of the whole ecosystem.

Also changed input publish to candidate because it feels weird to say "run hackage-publish but don't publish".

Tested here: https://github.com/fourmolu/fourmolu/actions/runs/4624388970/jobs/8203029673

@brandonchinn178 brandonchinn178 force-pushed the rewrite-js branch 16 times, most recently from fb91ddd to 2b42ba7 Compare April 7, 2023 00:17
@brandonchinn178 brandonchinn178 marked this pull request as draft April 7, 2023 00:17
@brandonchinn178 brandonchinn178 force-pushed the rewrite-js branch 10 times, most recently from 953ae04 to 1b9c8e1 Compare April 7, 2023 05:26
@brandonchinn178 brandonchinn178 marked this pull request as ready for review April 7, 2023 05:36
@brandonchinn178
Copy link
Member Author

@AlexeyRaga Is this package still maintained? If not, I'd be happy to take over.

@jamesdbrock
Copy link

What are the functional advantages of this PR @brandonchinn178 ? How does it behave better than the bash-based action?

@brandonchinn178
Copy link
Member Author

Three concrete functional advantages from just a quick skim of the bash code:

  • What if packagesPath contains a space? The find command will output a space and the for loop would iterate on them as separate files
  • What if the find command fails for some ephemeral reason? The for loop would iterate on an empty string and immediately return successfully
  • What if the curl command for HACKAGE_STATUS failed without getting a status? Maybe the package doesnt exist, but the network is flaky, causing HACKAGE_STATUS to be an empty string instead of 404, and then the script does something

Sure, maybe these arent terrible issues, and maybe the script is small enough to be easy to debug or whatever. But bash has so many small edge cases that can bite you later.

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