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

Resolve comment formatter issues #184

Merged
merged 3 commits into from
Apr 11, 2024
Merged

Resolve comment formatter issues #184

merged 3 commits into from
Apr 11, 2024

Conversation

hazendaz
Copy link
Member

@hazendaz hazendaz commented Apr 6, 2024

fixes #164

- inline license header is well known to be extremely buggy.  Exclude the output files as it will end up adding multiple headers.
- All items in the expected items are tab based, the tests are tab based, make sure only tabs are being output there including the license header.
- Pass preferences to the comment handler as is done with other handler
- Completely rewrite comment handler.  It does not need a pattern.

For the rewrite, here is how it handles
- For single line items, it will just run single indent as was original logic
- For <!-- only, --> only, or starts with <!--, it will strip leading spacing and add original indent as was original case.
- If indent comes in with no indention and line is physically not empty, then use the canonical indent ( these are headers at least here and need tabbed in with actual spacing not empty )
- For all others do a double indent.

For all here this works well and file output looks coherent in that it uses all same styling throughout.  The license plugin did trip this up, so that was fixed by ignoring them as it will conflict with all other files and double headers result.  So avoiding that outright.
@hazendaz
Copy link
Member Author

hazendaz commented Apr 6, 2024

This solution is a bit complicated with defect with license plugin involvement. So I don't expect any immediate merge on this. This also picked up a todo item that I see do nothing being added and I adjusted one set of strings from double quotes to single quotes but it didn't matter so I am going with todo says its useful so applied that too. From perspective here, it all looks good to me and formatter-maven-plugin tests now format properly. Previously those had mixture of spaces for license header and tabs for everything else. Now its all tabs as expected so its an improvement but I have concerns around effectiveness of this with license plugin since that results in double headers. But as noted on the issue ticket, that is a known issue with license plugin already and for a long time. That said, I don't know if we want to add more parameters to do it this way in best case but have some way to avoid the problems that may occur.

@hazendaz hazendaz self-assigned this Apr 11, 2024
@hazendaz hazendaz merged commit 84a2d88 into revelc:main Apr 11, 2024
6 checks passed
@hazendaz hazendaz added this to the 0.4.0 milestone Apr 11, 2024
@hazendaz hazendaz deleted the main branch April 11, 2024 20:46
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.

Multiline comments repeatedly indent
1 participant