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

feat: Introduce multiline_string_to_heredoc fixer #7665

Merged
merged 8 commits into from Jan 10, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jan 2, 2024

fix #7554

Multiline strings breaks indentation visually, this fixer fix such strings to heredoc/nowdoc and with heredoc_indentation the visual code flow is then much more better.

@mvorisek mvorisek marked this pull request as ready for review January 3, 2024 10:47
@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 3, 2024

PR is done. Tested on the whole repo, all unescaping should be done correctly.

@Wirone
Copy link
Member

Wirone commented Jan 3, 2024

@mvorisek I believe this fixer should be run after no_useless_concat_operator, so multiple, concatenated multi-line strings are not transformed into multiple heredocs, instead of one merged. Integration test for that should be added.

@mvorisek mvorisek marked this pull request as draft January 3, 2024 21:50
@mvorisek mvorisek force-pushed the multiline_string_to_heredoc branch 3 times, most recently from 966909b to 774c0e1 Compare January 7, 2024 00:56
@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 7, 2024

I believe this fixer should be run after no_useless_concat_operator, so multiple, concatenated multi-line strings are not transformed into multiple heredocs, instead of one merged. Integration test for that should be added.

This is not possible as this fixer must be run before escape_implicit_backslashes which has higher priority.

no_useless_concat_operator should however handle heredoc contatenations, so in the end, it should be runnable before/after - #7662

@mvorisek mvorisek marked this pull request as ready for review January 7, 2024 01:23
@mvorisek mvorisek requested a review from Wirone January 7, 2024 01:26
@Wirone
Copy link
Member

Wirone commented Jan 9, 2024

@mvorisek I know you want to help by keeping PRs up-to-date, but rebasing all of them does not make sense because after merging any of them the rest is outdated and they just waste CI capacity 😅. I do rebase after the review, so when it's approved and CI passes it gets merged. Parallel workflows take more time because some of the jobs are waiting for available runners. Let's do one after another 🙂.

Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Minor improvements 🙂.

Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

I don't know if I can foresee possible problems that could emerge with this particular implementation, but looks good enough as initial version, so I'm fine with it - we can improve/fix it later if users report something 🙂.

@Wirone Wirone force-pushed the multiline_string_to_heredoc branch from 50f365b to 17b3d8d Compare January 10, 2024 00:41
@Wirone Wirone changed the title feat: Add multiline_string_to_heredoc fixer feat: Introduce multiline_string_to_heredoc fixer Jan 10, 2024
@Wirone Wirone enabled auto-merge (squash) January 10, 2024 00:42
@Wirone Wirone merged commit 3705f91 into PHP-CS-Fixer:master Jan 10, 2024
25 checks passed
@mvorisek mvorisek deleted the multiline_string_to_heredoc branch January 10, 2024 00:46
@Wirone
Copy link
Member

Wirone commented Jan 10, 2024

Thank you @mvorisek 🍻!

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.

None yet

2 participants