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

feat(core): add getBooleanInput function #725

Merged
merged 3 commits into from Apr 28, 2021
Merged

Conversation

yi-Xu-0100
Copy link
Contributor

Gets the input value of the boolean type in the YAML specification.
The return value is also in boolean type.

ref: https://yaml.org/type/bool.html

close #723

@yi-Xu-0100 yi-Xu-0100 requested a review from a team as a code owner February 24, 2021 04:49
@yi-Xu-0100 yi-Xu-0100 changed the title feat: add getBooleanInput function feat(core): add getBooleanInput function Feb 24, 2021
@yi-Xu-0100
Copy link
Contributor Author

yi-Xu-0100 commented Feb 24, 2021

@ericsciple After I read the discussion in #362, I find that the YAML 1.2 core schema is implemented (#362 (comment)). So could we add the getBooleanInput function into the core package? 😀

Copy link
Collaborator

@thboop thboop 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 the contribution @yi-Xu-0100! I left some minor feedback before merging, happy to take the pr over if you don't have the time to continue working on this!

packages/core/README.md Outdated Show resolved Hide resolved
* @param name name of the input to get
* @returns boolean
*/
export function getBooleanInput(name: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export function getBooleanInput(name: string): boolean {
export function getBooleanInput(name: string, options?: InputOptions: boolean) {

Lets keep consistent with the getInput function. We really shouldn't throw if the value does not exist or is invalid, just return false. We can debug if the parsing does not match either case, to help with troubleshooting if users encounter this issue

We can add another option to InputOptions to set a default value, and return that instead of false if needed as well. I'm happy to tackle that in another pr if you want.

@yi-Xu-0100
Copy link
Contributor Author

yi-Xu-0100 commented Apr 28, 2021

@thboop Sorry for the delayed reply, I tried to fix the conflict with the main branch, but it didn't seem to get it right. 😅 So I pulled it again, attached my changes, and pushed it forcibly.

The change of what I did:

  1. I fixed the README descriptions, add a suggestion about the required option, and try to format the content.
  2. I add options for getBooleanInput function and use the getInput to deal with it.
  3. I add the support boolean input list into the type error.
  4. I add more tests for the getBooleanInput function.

For the error, I think the function is used to deal with boolean input, It should have a default value in action.yml if it is not necessary. And when it should be decided by users, it also should return a type error when users input the wrong value, which will help users to solve it without asking. What do you think? 😀

@yi-Xu-0100 yi-Xu-0100 requested a review from thboop April 28, 2021 03:50
@thboop
Copy link
Collaborator

thboop commented Apr 28, 2021

And when it should be decided by users, it also should return a type error when users input the wrong value, which will help users to solve it without asking.

Agreed, thanks for the contribution!

Copy link
Collaborator

@thboop thboop 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 contribution @yi-Xu-0100!

const val = getInput(name, options)
if (trueValue.includes(val)) return true
if (falseValue.includes(val)) return false
throw new TypeError(

Choose a reason for hiding this comment

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

what if it was required: false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chamini2 The required: false will be dealt with the getInput(Line 107) and get the default value. 👀

Choose a reason for hiding this comment

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

But if there is no default value, that will make val an empty string, which we don't consider in line 108 or 109. Resulting in throw new TypeError because trueValue.includes('') and falseValue.includes('') are both false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Action inputs can be read with getInput which returns a string or getBooleanInput which parses a boolean based on the yaml 1.2 specification. If required set to be false, the input should have a default value in action.yml.

https://github.com/yi-Xu-0100/toolkit/blob/main/packages/core/README.md

@chamini2 Why a boolean type input have no default value? 👀

Choose a reason for hiding this comment

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

Oh, I did not know this was expected. If required: false and no default I thought it had just to return null or ''. Thanks, sorry for the inconvenience.

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.

[feature] Add new getBooleanInput function for the core
4 participants