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

Convert all throws into Results #20

Closed
wants to merge 25 commits into from
Closed

Convert all throws into Results #20

wants to merge 25 commits into from

Conversation

lucperkins
Copy link
Member

@lucperkins lucperkins commented Apr 23, 2024

A small quality of life thing but one that should have some ergonomic benefits across our Actions. Basically makes this lib more like Rust. No more try/catch, just Result<T, string> (where the string is error text). There are still try/catch blocks in this library, of course, but nothing here throws errors. I've also streamlined a few other things, like assigning main and post hooks to IdsToolbox.

You can see an example in DeterminateSystems/nix-installer-action#81

Copy link

netlify bot commented Apr 23, 2024

Deploy Preview for detsys-ts-docs ready!

Name Link
🔨 Latest commit 7420768
🔍 Latest deploy log https://app.netlify.com/sites/detsys-ts-docs/deploys/663972625484c9000802eb3f
😎 Deploy Preview https://deploy-preview-20--detsys-ts-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lucperkins lucperkins requested a review from grahamc May 2, 2024 17:27
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

I've gotta say I'm not real thrilled about this PR, because it seems weird. Is this a thing that a lot of JS projects are doing? I don't want to be weird here.

Comment on lines +163 to +164
if (options.debug)
actionsCore.debug(`Trying to read '${osReleaseFile}'...`);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (options.debug)
actionsCore.debug(`Trying to read '${osReleaseFile}'...`);
if (options.debug)
actionsCore.debug(`Trying to read '${osReleaseFile}'...`);

-1 on the braceless if's

@lucperkins
Copy link
Member Author

Not a lot of projects do it, no, but throws and try/catch blocks are completely and utterly inscrutable and untraceable in JS. Anything can throw an error at any time and there's no guarantee that that error is properly handled or even surfaced in a way that indicates where it came from. This provides us with full control of the error handling "stack" in a way that JS on its own does not afford.

@lucperkins
Copy link
Member Author

And not a lot of projects do it because this is widely seen as one of those this-is-fine.png aspects of working with JS. Just a kind of brute fact we all have to suffer under. It's up to you but I'd like to see us reject that "fact."

@lucperkins
Copy link
Member Author

The library is much more well structured now, so I'm less concerned about stray throws.

@lucperkins lucperkins closed this May 22, 2024
@lucperkins lucperkins deleted the ts-results branch May 22, 2024 19:39
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.

None yet

2 participants