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 the JSON syntax error of running phpcd-diff action when no changes are made #27

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

Fix the JSON syntax error of running phpcd-diff action when no changes are made.

For some reason, if runs phpcs-diff action without any file changes, it will get a JSON syntax error because the vendor/bin/diffFilter outputs an empty result.

image

Detailed test instructions:

  1. Create a local bash file at the repo's root directory temporality to verify the changes of this PR.
    #!/usr/bin/env bash
    JSON_REPORT="/tmp/phpcs-diff.json"
    
    # The original command
    # vendor/bin/diffFilter --phpcsStrict <(git diff HEAD^...HEAD) <(vendor/bin/phpcs ./* -q --report=json) --report=phpcs 0 > "$JSON_REPORT"
    
    # Simulate the result of no changes - an empty file.
    printf '' > "$JSON_REPORT"
    
    # Simulate the result of no PHPCS errors.
    # printf '{"files":[],"totals":{"errors":0,"fixable":0,"warnings":0}}' > "$JSON_REPORT"
    
    # Simulate the result of a PHPCS error.
    # printf '{"files":{"woocommerce-google-analytics-integration.php":{"messages":[{"message":"Expected 1 spaces before closing parenthesis; 0 found","source":"diffFilter","severity":1,"type":"ERROR","line":23,"column":1,"fixable":"false"}],"errors":1,"warnings":0}},"totals":{"errors":1,"fixable":0,"warnings":0}}' > "$JSON_REPORT"
    
    if [ ! -s "$JSON_REPORT" ]; then
      echo "No changes."
      exit 0
    fi
    
    node packages/js/github-actions/actions/phpcs-diff/src/annotate-phpcs-report.js "$JSON_REPORT"
  2. Run the verification bash file with different diff results to see if the fix works well.

@eason9487 eason9487 requested a review from a team July 8, 2022 08:13
@eason9487 eason9487 self-assigned this Jul 8, 2022
@tomalec
Copy link
Member

tomalec commented Jul 8, 2022

To me it looks like a bug in diffFilter (exussum12/coverageChecker#72), as the line https://github.com/exussum12/coverageChecker/blob/495c9cb512cdb057cd2c63e6bda0efee4838bb3a/src/functions.php#L94-L95 logs an error, but does not throw, and returns with success 0 code.

IMO, it should either return 1 if the error occurred, and it cannot proper the result in a compliant format, or return the result in the compliant format (JSON)

Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

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

Tested locally LGTM.
💅 We may add a link to the issue in the code comment, to maybe someday be able to remove this workaround :)
(the repo is not very active, but there is always hope;) )

@eason9487
Copy link
Member Author

@tomalec, thanks for reporting the issue!

We may add a link to the issue in the code comment, to maybe someday be able to remove this workaround :)

Sounds good! I added in 7d69c1e.

@eason9487 eason9487 merged commit 5567a0a into trunk Jul 11, 2022
@eason9487 eason9487 deleted the fix/phpcs-diff-no-changes branch July 11, 2022 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants