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

Input includes as an array (Fix #16) #22

Merged
merged 7 commits into from
May 28, 2024
Merged

Conversation

SherbetLemon47
Copy link
Contributor

Summary

Describe the goal of this PR. Mention any related Issue numbers.
Fixes #16.

Requirements (place an x in each [ ])

Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 89.36170% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 64.57%. Comparing base (339d493) to head (7483376).
Report is 2 commits behind head on main.

Files Patch % Lines
src/score_components/find-problematic-comments.js 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   56.76%   64.57%   +7.80%     
==========================================
  Files           5        6       +1     
  Lines         229      271      +42     
==========================================
+ Hits          130      175      +45     
+ Misses         99       96       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SherbetLemon47
Copy link
Contributor Author

@filmaj Please review

@filmaj filmaj self-requested a review May 18, 2024 12:40
@filmaj filmaj added the semver:minor addressing/merging the change would necessitate a minor semver release label May 18, 2024
@filmaj
Copy link
Contributor

filmaj commented May 18, 2024

@SherbetLemon47 I'm not sure that this approach works w/ YAML arrays/lists. Check out this article and scroll down to "Defining Arrays in YAML." Ideally we should support either block or flow arrays in YAML for the include, exclude and extension inputs of this Action. Oh, and, as always, will require unit tests covering this functionality 🙇

Copy link
Contributor Author

@SherbetLemon47 SherbetLemon47 left a comment

Choose a reason for hiding this comment

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

Added some tests, I suppose the new approach works both types of YAML arrays. Reworked comment function to support extensions arrays. For a better YAML to JSON approach I think would have to go for a library.

This reverts commit 3f67dba.
@SherbetLemon47
Copy link
Contributor Author

SherbetLemon47 commented May 21, 2024

Even though I pushed a commit. I've come to find that github actions doesn't actually support arrays as input. Source- Here. Wrapping the strings in double quotes does work i.e things like "['src','test']", etc. but I don't think a proper array implementation is possible But the approach mentioned above, the one with multiline strings does work with this.

@SherbetLemon47
Copy link
Contributor Author

@filmaj waiting for a review, moved stubs to a separate file, so further testing on other features would require merging or discarding the PR.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Great work! I left some comments, mostly about refactoring, organization and some test questions, but overall looking great. Thank you so much for working on this!

test/stubs/stubs.js Show resolved Hide resolved
src/health-score.js Outdated Show resolved Hide resolved
src/health-score.js Outdated Show resolved Hide resolved
src/health-score.js Outdated Show resolved Hide resolved
test/health-score-test.js Outdated Show resolved Hide resolved
test/health-score-test.js Show resolved Hide resolved
@SherbetLemon47
Copy link
Contributor Author

SherbetLemon47 commented May 28, 2024

Added all the requested changes and some really overkill tests. Please review. @filmaj

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Very nicely done! Many thanks for your hard work here 🙇

@filmaj filmaj merged commit 4567da0 into slackapi:main May 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed semver:minor addressing/merging the change would necessitate a minor semver release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'include' multiple folders
2 participants