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

fix boolean handling #140

Merged
merged 2 commits into from
Jan 8, 2025
Merged

fix boolean handling #140

merged 2 commits into from
Jan 8, 2025

Conversation

theoephraim
Copy link
Contributor

fixes 2 issues, although you may want to adjust how its done...

1 - optional booleans were throwing an error when empty. Not sure if you want to return undefined, or just default to false if empty.

2 - boolean defaults were causing issues when unquoted in the yaml file

@theoephraim theoephraim requested a review from ncalteen as a code owner January 8, 2025 00:15
@theoephraim theoephraim force-pushed the fix-boolean-handling branch from 2491fa0 to 973eaea Compare January 8, 2025 00:17
@ncalteen
Copy link
Collaborator

ncalteen commented Jan 8, 2025

Copying my comment from the issue thread:

FYI the GitHub YAML schema expects a string value. This is also documented in the action syntax documentation.

Additionally, I will need to double check what @actions/core is returning for optional, undefined boolean inputs. Bear with me and I will check on this first thing tomorrow!

@theoephraim
Copy link
Contributor Author

theoephraim commented Jan 8, 2025 via email

@ncalteen
Copy link
Collaborator

ncalteen commented Jan 8, 2025

To be extra pedantic, I ran the following tests to confirm behavior:

Required Default Value Type Input Value Type Result
true string None OK
true None string OK
true bool None OK
true None bool OK
false string None OK
false None string OK
false bool None OK
false None bool OK
false None None Input does not meet YAML 1.2 "Core Schema" specification

So it looks like the schema itself is not really enforced. The only one that should error out is an optional input with no value. In that case, I believe the scenario you pointed out in issue 1 should still throw a Input does not meet YAML 1.2 "Core Schema" specification error to remain consistent with the default @actions/core functionality, but issue 2 is valid and needs to be resolved.

src/stubs/core/core.ts Outdated Show resolved Hide resolved
src/stubs/core/core.ts Outdated Show resolved Hide resolved
@ncalteen
Copy link
Collaborator

ncalteen commented Jan 8, 2025

Can you also bump the version in package.json to 2.5.1? That way when this PR is merged, it will release a new version to npm.

@theoephraim
Copy link
Contributor Author

theoephraim commented Jan 8, 2025

re: throwing an error - If the item is marked as optional, it seems unexpected to throw an error when empty. It's effectively saying there is no such thing as an optional boolean. Either coercing to false or leaving it empty/undefined seem reasonable to me, but throwing an error doesn't feel quite right. If I want to have an optional boolean input, I would now have to wrap getting the value in a try/catch.

re: versioning - I can bump this one, but I'd strongly recommend setting up some automation with either https://github.com/changesets/changesets or https://www.npmjs.com/package/release-it.
edit - I see you do have some automation here, but still it's nice to not update the version numbers manually. Changesets has been really nice to work with...

@ncalteen
Copy link
Collaborator

ncalteen commented Jan 8, 2025

re: throwing an error - If the item is marked as optional, it seems unexpected to throw an error when empty. It's effectively saying there is no such thing as an optional boolean. Either coercing to false or leaving it empty/undefined seem reasonable to me, but throwing an error doesn't feel quite right.

I don't disagree, but that would be a change that needs to happen in the @actions/toolkit side first, since the goal of this project is to replicate the same behavior.

@theoephraim
Copy link
Contributor Author

ah of course - that makes sense. Forgot for a sec what this tool was actually doing 😅

Will make changes and should be ready to go

Copy link
Collaborator

@ncalteen ncalteen left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 Thanks for the support!

@ncalteen ncalteen merged commit ccda8ef into github:main Jan 8, 2025
4 checks passed
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