Add ability to track the location of cells that raise casting exceptions #85
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using a default implementation of castValue(…) with row and column numbers we can silently add the ability to record the location of any casting being done for logging purposes.
This is an example implementation of the newly modified Casting interface, which stores information about casting errors for later retrieval.
|
Hi @brunslo , thank you for your effort! here we need to improve our coverage more to be able to accept this PR |
ozlerhakan
reviewed
Jun 18, 2019
Fix regression due to overzealous IDE.
Given that the original authors were open to logging location in Casting, it makes sense to reduce code duplication by adding the logging code directly to DefaultCasting. Logging is disabled by default and enabled with a boolean flag to the constructor. A new public method isErrorLoggingEnabled() is added, which indicates whether or not logging was enabled on creation. A new public method getErrors() is also added that returns details of any exception thrown during a cast, including the sheet, row, and column. This method throw an IllegalStateException when called on an instance without logging enabled. This ensures that a returned empty getErrors() List when logging is disabled is not misinterpreted to mean there were not casting exceptions thrown during unmarshaling. Existing tests for DefaultCasting now include a check to ensure that logging is disabled and that the expected IllegalStateException is thrown when attempting to call getErrors(). Another set of tests that interrogates the logging will be in a forthcoming commit.
|
I took the presumptive step to fold the logging additions into the I still have the |
|
Thank you @brunslo , I'll look at the code you made 🎉 |
|
merged @brunslo ! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With a backwards compatible change to the Casting interface it is simple to add the ability to log formatting errors found during the casting of values from the original workbook cells. The change adds a default method extending
castValuewhich accepts row and column numbers along with the regular casting parameters. This change, coupled with the passing of already known row and column numbers inHSSFUnmarshallerandPoijiHandlerinto the few placescastValueis called gives a light weight error location tracker for very little cost.An example of how this could be done is provided in the
LogCastingclass, which is provided as a simple proof of concept without tests.