GH-30: Handle files that are empty, have an unknown newline, or can't…

[dwalluck]

… be parsed

[dwalluck]

In addition to throwing a special subclass of IOException on empty files as mentioned in GH-30, this handles two other cases: throw a special exception on UNKNOWN newline, and a normal IOException on parse failure of Java code. The existing code failed to check parseResult.isSuccessful() and in this case, a CompilationUnit still gets returned, so files with syntax errors in the Java code were not being caught. I am not really sure what happened to the files in the case where parseResult.isSuccessful() is false but the code continues past this.

I understand that the Mojo already has access to the file path, so it may not be necessary to store the Path in the exception. However, using separate exceptions allows us to handle different cases easier (e.g., we print a warning instead of failing in two of the three cases).

[ctubbsii]

Thanks for the PR, @dwalluck
I think these changes are fundamentally good, but I think the code in the PR could be polished up a bit. If you want, I can do the polishing myself, but I made a few suggestions, if you want to do it and update your PR.

[dwalluck]

Thanks for the PR, @dwalluck
I think these changes are fundamentally good, but I think the code in the PR could be polished up a bit. If you want, I can do the polishing myself, but I made a few suggestions, if you want to do it and update your PR.

I think I made all requested changes. Please check. I have added to the test cases an additional check which checks the exception message, to verify that the exception message is no longer null.

[ctubbsii]

@dwalluck Thanks for the PR. I merged your changes, then added some of my own modifications on top of them, that I liked better.

The one thing I didn't keep is the skipping of the files with errors. After reviewing your changes in depth, I decided it would be better to go ahead and fail at the end, if it saw any of these failure cases as well. If it is necessary to allow the code to ignore such failures, we can add a configuration options to ignore failures, and to exit normally from the plugin execution. This is aligned with what other plugins do, such as maven-surefire-plugin. Combined with the existing functionality to add excludes, I think that should suffice for all use cases.

Let me know what you think, and whether you'd be interested in contributing the extra configuration option to ignore failures, or if I should do it, or if you have a different opinion entirely. Thanks.

[dwalluck]

Let me know what you think, and whether you'd be interested in contributing the extra configuration option to ignore failures, or if I should do it, or if you have a different opinion entirely. Thanks.

When you combine this with a general excludes option that allows you to exclude files (maybe for some reason you don't wish to run it on a certain file, say, if it was copied from another codebase, etc.), then it makes sense.

The problem I see is that an empty Java file and a one line file is legal Java code and doesn't cause a compiler error. In the case of impsort, if there are no imports, then there's nothing for it to do and no reason why it should fail.

My personal opinion is that if the compiler itself accepts a file and there's nothing for impsort to do (say, if isSorted() is true), then it should at most be a warning and not fail the build.

I agree that in cases where impsort is prevented from doing its job, because newlines are inconsistent, then it makes sense to fail.

[ctubbsii]

My personal opinion is that if the compiler itself accepts a file and there's nothing for impsort to do (say, if isSorted() is true), then it should at most be a warning and not fail the build.

I agree with this... mostly. But, there are limits. For example, the parser library we use isn't perfect... and this plugin can have also have its own issues if the file isn't well-formatted first (as in #1). So, not all files accepted by the compiler are expected to work in this plugin. It expects some sanely-formatted files first, as a prerequisite.

Also, some of these situations may occur because of issues where something unexpected is occurring, and not because the file is actually intended to be zero-length or be one-line without a line ending (which are pretty unusual circumstances, after all). Some possibilities include: a bad mount point, an incorrect symlink, a file locked by an editor, etc.

Always failing on these files unless they are added explicitly to an excludes list I think is reasonable behavior, given the rarity of which these situations are expected to occur, and because they are far more likely indicative of an underlying problem, IMO, rather than an intended zero-length or one-line Java file.

I agree that in cases where impsort is prevented from doing its job, because newlines are inconsistent, then it makes sense to fail.

I like this way of presenting the problem. I'll get some dinner, and think it over a bit, and take another pass later today, or tomorrow.

[dwalluck]

I like this way of presenting the problem. I'll get some dinner, and think it over a bit, and take another pass later today, or tomorrow.

I also think that if you implement a general excludes option (as you said, most plugins have this option), then you may not need a separate option to ignore failures, since a user could just add that file to the excludes list in this case, right?

[ctubbsii]

I also think that if you implement a general excludes option (as you said, most plugins have this option), then you may not need a separate option to ignore failures, since a user could just add that file to the excludes list in this case, right?

Right, but we already have that excludes option here. I assumed, based on this conversation, that you were looking for more than that.

[dwalluck]

Right, but we already have that excludes option here. I assumed, based on this conversation, that you were looking for more than that.

It depends whether you think the existing option is good enough or there should be a separate one.

[ctubbsii]

Right, but we already have that excludes option here. I assumed, based on this conversation, that you were looking for more than that.

It depends whether you think the existing option is good enough or there should be a separate one.

What would you want a separate one to do? Exclude by category (empty, unknown line ending, etc.), or something else? I'm not sure what the "good enough" criteria is here. Personally, I think the current one is good enough... but maybe I'm missing a use case to consider.

[dwalluck]

What would you want a separate one to do? Exclude by category (empty, unknown line ending, etc.), or something else? I'm not sure what the "good enough" criteria is here. Personally, I think the current one is good enough... but maybe I'm missing a use case to consider.

The excludes option is probably "good enough" already. If you wanted to improve it, I think I would add separate boolean options for each case such as <failOnEmptyFile>true</failOnEmptyFile>.

[ctubbsii]

@dwalluck I made some changes in #34 that I think which covers some of the stuff we've discussed here. Specifically, it skips empty files, and only throws an exception on UNKNOWN line endings if the line ending was configured to KEEP. I changed the way it read in the lines to the array, so it wouldn't hit the NPE with UNKNOWN by trying to split the buffer on a null regex, but instead would use Java's built-in Scanner utility to read lines. Let me know on that PR what you think.

[ctubbsii]

I discovered a problem with checking isSuccessful and reported it upstream at javaparser/javaparser#2820

I have a workaround in progress.

[ctubbsii]

Worked around the above issue in 340d7e2

[ctubbsii]

The above workaround was removed and properly fixed by adding a configurable compiler compliance level #37