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: parse checkboxes #21

Merged

Conversation

elhmn
Copy link
Contributor

@elhmn elhmn commented Apr 8, 2022

This pull request allow checkboxes values to be parsed and stored in an array as described in #20

@stefanbuck
Copy link
Owner

Hi @elhmn, thanks for submitting this change. I do agree that the current output isn't perfect. From a quick glance it all make sense. I need some more time to review this in details since it's a breaking change. Therefore, please bear with me. Thanks in advance.

@stefanbuck stefanbuck force-pushed the parse-and-stores-checkboxes-entries-in-an-array branch from 8128e6b to c87aec2 Compare May 4, 2022 20:11
Copy link
Owner

@stefanbuck stefanbuck left a comment

Choose a reason for hiding this comment

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

Sorry that it took me so long to come back to this. I updated the README and made a tiny change. Apart from that I left a comment regarding a change which affects the action output. However, overall this change looks good. As said I'll look into the test for the output, maybe separately.

index.js Outdated
});

result = toObject(result);
core.setOutput(`issueparser`, result);
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if this change was intended. Right now every form value becomes its own output e.g issueparser_ email = [email protected]. However, this change will result in a single output named issueparser which will include the entire parsed response as a json string, which is the same as the current existing jsonString output.

Original I added this output per form value, to avoid the write/read file dance in case only value is needed. I think that way it is easier to work with that a json-string that needs to be parse ... What do you think @elhmn?

Copy link
Owner

Choose a reason for hiding this comment

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

Also interesting that none of the unit tests was catching that issue. The mock is looking for issueparser_ ... I'll look into this

@stefanbuck stefanbuck force-pushed the parse-and-stores-checkboxes-entries-in-an-array branch from ee7fae7 to 72eec4b Compare May 24, 2022 22:00
@stefanbuck stefanbuck changed the base branch from main to v3 August 19, 2022 22:26
@stefanbuck stefanbuck merged commit 3117a12 into stefanbuck:v3 Aug 19, 2022
stefanbuck added a commit that referenced this pull request Aug 29, 2022
* feat: parse checkboxes

* fixup! feat: parse checkboxes

* docs: Update readme to match v3 behaviour

* fix: ensure every value sets an output parameter

* chore: update readme

Co-authored-by: Stefan Buck <[email protected]>
stefanbuck added a commit that referenced this pull request Sep 19, 2022
* feat: parse checkboxes

* fixup! feat: parse checkboxes

* docs: Update readme to match v3 behaviour

* fix: ensure every value sets an output parameter

* chore: update readme

Co-authored-by: Stefan Buck <[email protected]>
stefanbuck added a commit that referenced this pull request Oct 19, 2022
* feat: parse checkboxes

* fixup! feat: parse checkboxes

* docs: Update readme to match v3 behaviour

* fix: ensure every value sets an output parameter

* chore: update readme

Co-authored-by: Stefan Buck <[email protected]>
@stefanbuck
Copy link
Owner

This has been finally landed in v3. Thanks for your patience with this release

@skhalaf21
Copy link

@stefanbuck , we noticed that although we use v2, the action parses checkboxes the new way, which caused our code to fail until we adjusted it to handle the new output format. Question is, how would this v3 impact v2 at all? we do specific v2 (and it has working fine for us until 7 days ago (10/26/22).

parse issue body

  - uses: stefanbuck/github-issue-parser@v2

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.

3 participants