Multiline comments repeatedly indent about xml-formatter HOT 9 CLOSED

Excellent @hazendaz and I'm apologize for the delay. I'm more behind on emails than I thought I was :)

If you would like me to review or test anything, please let me know. I'm happy to help.

No worries at all. Initially I wasn't sure how much was involved to reproduce using our own code. Turns out very little. Further turned out our tests should have been super obvious this was a problem to start with. I hope to get some cycles this weekend to square this away as I really want to get this included and released with next formatter plugin release set. I was close last weekend but have to look further out in the code as license headers in our tests are mixed (spaces) yet everything else wants tabs. And for whatever reason the logic I rewrote works great except those headers come in and seem to say use a single space not a tab which causes it to be off. Once I do get that squared away would love for you to review it. I know I'm close but haven't had time to come back to it yet.

from xml-formatter.

@jamezp If you are to rebuild the current master and try using that, does this issue still show up? If so, would you mind putting together a little reproducible repo that we can take further look at. Thanks.

from xml-formatter.

@jamezp No need for reproducible. I have added a comment into the testing over on formatter-maven-plugin that uses this module and have reproduced this. The issue happens on current master as well as prior release.

from xml-formatter.

@jamezp I'm working on a fix. I've rewrote the comment handler for modern usage which further fixes fact that spaces and tabs being mixed were not being properly set per requested formatting within comments. For example, out current tests are set to spaces in that area but then tabs are expected during testing but it leaves spaces. The fixes I'm working on correct that so tabs are used. This is tabs to the start of comments not spaces within them. This further shows a few things like if we have multi line comments that could be single based on line size they are wrongly treated now. And further, if we have multi line we should be properly formatting them to break as expected per xml standards and we fail to do so. Those issues I'm not addressing right now but believe that the caller into the comment logic is 100% wrong on the headers as I'm getting single space instead of tabs during testing and that results in the headers getting stripped entirely of setup but all else is working. So it will be a week or two maybe before I get this resolved. I hope to get this into next formatter maven plugin release.

from xml-formatter.

Excellent @hazendaz and I'm apologize for the delay. I'm more behind on emails than I thought I was :)

If you would like me to review or test anything, please let me know. I'm happy to help.

from xml-formatter.

@jamezp Can you rebuild from my PR #184 and see if that solves your issues. I've tried it with formatter-maven-plugin and it improves there too fixing license headers clearly applied incorrect with spaces where tabs were requested. Its a bit complicated on what had to change but gist is I rewrote the handler entirely so here is how its working and with no regex.

The caller in all cases is stripping out indents well before the snippet so its only dealing with the snippet (tag block). An indent is required after stripping concerns so each case has that indent added which was the same as the original.

The processing otherwise is as follows

• [same] For lines to format that is < 2 meaning 1 line, it is same as it was before. It just adds the required indent as sent in from caller. This could have fell through and still worked the same but it does reduce checks to get out early.
• [new] For line that is exactly , or contains <!-- (later one could be and probably should be line broke but left that part). In this case, it adds the indent and strips any leading spaces. The logic is same as before but check is slightly different so its more accurate.
• [new] If indent passed in is completely empty and line is NOT empty (likely license header), it will use the canonicalIndent. This does two things. In the existing code it was adding spaces always even if tabs asked for which was wrong. This corrects that. Further since it sends in no indent it ensures it actually is indented and not against the left margin. The changes without this were leaving header only at left margin so this ensures that is not the case.
• Finally all other elements in that tag get two indents.

So what you end up with is results like the following

	<!--
		Here's a comment block.
		Make sure they have correct indentation after format.
	-->

	<!--
		Here's a comment block with leading spaces.
		Make sure they have correct indentation and keep the leading spaces after format.
	-->

	<!--Here's a comment block with unaligned block start/end.
		Make sure they have correct indentation after format.-->

As you can see the one that I think is still entirely wrong is the last one. I think in that case we should break that to match those of the first two as its far cleaner. But that adds more complexity and I think we need to just test this more thoroughly to make sure its behaviour is as expected. I don't have much to test this with so any help would be appreciated to see that it fixes your issues and does not regress.

The one additional thing I had to do here is make the license plugin skip the output files. Unfortunately its well known that the license plugin has serious issues if any changes to output on the inline usage. I work on that project too but haven't figured out the issue there yet. So that had to be ignored here. I don't use inline myself anywhere but this project does recently which I think is bad form but is what it is. So I expect if that is your situation too you will not have a positive result as it will duplicate the headers. I don't think though that should really be considered as an issue here though. Other activities really should not be a responsibility of this plugin and given I can see that happening that will help trying to resolve the license plugin issue. You can see that specific issue here mathieucarbou/license-maven-plugin#153, although closed it was disputed as recently as February as a problem and I've also seen it happen from time to time depending on setup.

from xml-formatter.

Excellent thank you @hazendaz. I built this project and the formatter-maven-plugin with the upgrade. This fix works for the issue I was seeing.

I do agree on the point of the last comment format as well. It does seem it should match, but if it's complicated I don't think it's a huge deal. I haven't ran into the issue personally :)

I don't use the license-maven-plugin so I unfortunately can't comment much there.

from xml-formatter.

Thanks for checking it out @jamezp. I'm hoping to progress things enough in coming weekend to get a release out. Will wait until this weekend before I merge this.

from xml-formatter.

Excellent. Thank you @hazendaz!

from xml-formatter.