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: Convert literal \n to actual newlines in GitHub CLI interactions #366

Merged
merged 1 commit into from
Mar 24, 2025

Conversation

bhouston
Copy link
Member

Description

This PR fixes the issue where GitHub comments created by mycoder sometimes don't escape newlines properly, resulting in literal "\n" strings in PR descriptions instead of actual line breaks.

Changes

  1. Added preprocessing in both shellExecute and shellStart tools to convert literal \n strings to actual newlines before base64 encoding the content.
  2. Added unit tests to verify this functionality.
  3. Created documentation (docs/github-cli-usage.md) explaining the proper way to handle multiline content with GitHub CLI.

Related Issues

Fixes #365

Testing

  • Added unit tests that verify literal \n strings are properly converted to actual newlines.
  • Tested both shellExecute and shellStart functionality.

Documentation

Added a new documentation file docs/github-cli-usage.md with best practices for using GitHub CLI with multiline content.

@bhouston
Copy link
Member Author

PR Review: Fix for GitHub Comments Newline Handling

Issue Alignment

  • ✅ The PR directly addresses issue PR description test escaping issue #365 regarding the improper handling of newlines in GitHub comments.
  • ✅ The solution is focused and tackles exactly what was requested in the issue.
  • ✅ The PR considers the points raised in the issue discussion about using stdinContent with GitHub CLI.

Code Quality

  • Clean Design: The implementation is simple and effective, using straightforward regex replacements.
  • Terseness: The code changes are minimal and focused, adding just a few lines to handle the conversion.
  • Consistency: The same approach is implemented in both shellExecute.ts and shellStart.ts.
  • Naming: Variable names are clear and consistent with the existing codebase.

Function Design

  • Single Responsibility: The added code has a clear purpose: converting literal newlines to actual newlines.
  • Side Effects: The transformation is contained and only affects the stdinContent parameter.
  • ✅ The change is made at the right location in the code flow, just before the content is encoded to base64.

Testing

  • ✅ Appropriate unit tests have been added for both shellExecute and shellStart.
  • ✅ The tests verify that literal \n strings are properly converted to actual newlines.
  • ✅ The tests use spy functions to capture and verify the transformation.

Documentation

  • ✅ A new documentation file docs/github-cli-usage.md has been added.
  • ✅ The documentation clearly explains how to use GitHub CLI with multiline content.
  • ✅ Good examples are provided for both template literals and escaped string approaches.
  • ✅ Best practices and troubleshooting tips are included.

Performance & Security Considerations

  • ✅ The regex replacements are simple and efficient, with no performance concerns.
  • ✅ No security issues are introduced by these changes.

Overall Assessment

This PR provides a well-implemented fix for the issue with GitHub comments not properly handling newlines. The changes are minimal, focused, and well-tested. The added documentation is clear and comprehensive.

Recommendations

  1. Approve: I recommend approving this PR as it provides a clean, focused fix for the issue.
  2. Documentation Reference: Consider adding a reference to the new github-cli-usage.md document in the main README or CONTRIBUTING guide to make it more discoverable.
  3. Real-world Testing: While the unit tests are good, it would be beneficial to verify with a real-world test that creating GitHub issues/PRs with multiline content now works correctly.

@bhouston bhouston merged commit f165a25 into main Mar 24, 2025
2 checks passed
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.

PR description test escaping issue
1 participant