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

ComposerLockDiff changes line endings, breaking editing PR descriptions in GitHub #318

Closed
deviantintegral opened this issue Oct 28, 2023 · 8 comments
Assignees
Labels
bug Something isn't working client affected

Comments

@deviantintegral
Copy link
Member

When editing pull request descriptions, ComposerLockDiff changes \r\n to \n\n. This makes it very difficult to edit the PR description in the UI, because you have hidden unprintable characters. Top is before (pull_request.json), bottom is after (processed.json):

image

This would be partially solved by #317 moving the contents to a comment, but should also be fixed because \n is not supported: https://stackoverflow.com/a/46751663 (most web applications use \r\n for line endings): https://www.rfc-editor.org/rfc/rfc7230#section-3.5

@justafish
Copy link
Member

Closing in favour of #332

@deviantintegral
Copy link
Member Author

@justafish I think this still needs to be fixed separately from moving to a comment, because \n\n is not a valid line ending character in web applications. Otherwise, we could have undefined or broken behaviour in the rendering of the changes.

Something I missed here?

@beto-aveiga
Copy link
Collaborator

If we are going to do this, I can work on a PR to fix the \n\n \r\n issue.
@deviantintegral @justafish

@beto-aveiga beto-aveiga self-assigned this May 20, 2024
@YesCT
Copy link
Contributor

YesCT commented May 20, 2024

@beto-aveiga I'm not sure, but I think you might want to build on the branch from #538 . Unless @davereid is also going to work on the line ending thing in his PR.

@beto-aveiga beto-aveiga removed their assignment May 21, 2024
@davereid
Copy link
Member

My fix is to move the composer lock diff to a sticky PR comment instead of updating the PR description. The work for this would be fixed by #538

@davereid
Copy link
Member

But we could put in a fix now since that's needing some more detailed time and reviews. Ultimately the fix would be deprecated/removed by the work in #538.

@mrdavidburns
Copy link
Member

Going to move this into Blocked until we move this task out of DDEV.
Complete #283 first

@justafish
Copy link
Member

Fixed in #636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client affected
Projects
None yet
6 participants