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

Enhance pr_changed_files with exit codes and string output #151

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

iangmaia
Copy link
Contributor

@iangmaia iangmaia commented Feb 12, 2025

Follow up to the discussion started on woocommerce/woocommerce-ios#15086 (comment).

This PR adds both exit codes (default mode) and "true" "false" output (using --stdout flag) to the pr_changed_files command, updating documentation and tests accordingly.


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@iangmaia iangmaia requested a review from AliSoftware February 12, 2025 23:32
@iangmaia iangmaia self-assigned this Feb 12, 2025
@dangermattic
Copy link

dangermattic commented Feb 12, 2025

1 Warning
⚠️ Please add an entry in the CHANGELOG.md file to describe the changes made by this PR

Generated by 🚫 Danger

bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
tests/pr_changed_files/test_helpers.sh Outdated Show resolved Hide resolved
tests/pr_changed_files/test_all_match_patterns.sh Outdated Show resolved Hide resolved
@iangmaia iangmaia marked this pull request as ready for review February 13, 2025 11:08
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Outdated Show resolved Hide resolved
bin/pr_changed_files Show resolved Hide resolved
Comment on lines 66 to 69
# $1 - Expected return code
# $2 - Actual return code
# $3 - Actual output
# $4 - Expected output
Copy link
Contributor

Choose a reason for hiding this comment

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

Order of arguments feels a bit strange, with Expected and Actual being intertwined like that.
I'd suggest [Expected return code, Actual return code, Expected output, Actual output] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed -- updated on feb51f0

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