-
Notifications
You must be signed in to change notification settings - Fork 101
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
Expose exceptions in DumpProcessingController #526
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #526 +/- ##
============================================
+ Coverage 81.02% 81.07% +0.04%
- Complexity 2085 2128 +43
============================================
Files 149 148 -1
Lines 7293 7354 +61
Branches 895 902 +7
============================================
+ Hits 5909 5962 +53
- Misses 1115 1122 +7
- Partials 269 270 +1 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks globally fine to me. I just have two improvement suggestions
@@ -374,7 +374,8 @@ public Sites getSitesInformation() throws IOException { | |||
* @see DumpProcessingController#processDump(MwDumpFile) | |||
* @see DumpProcessingController#getMostRecentDump(DumpContentType) | |||
*/ | |||
public void processAllRecentRevisionDumps() { | |||
public void processAllRecentRevisionDumps() | |||
throws IOException, FileAlreadyExistsException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileAlreadyExistsException
is a sub class of IOException
(same in other places)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there is still a case for including both of them explicitly in the signature, because the caller might want to handle them differently?
+ " already exists. Try deleting the file or dumpfile directory to attempt a new download."); | ||
+ " already exists. Try deleting the file or dumpfile directory to attempt a new download."; | ||
logger.error(errorMessage); | ||
throw new FileAlreadyExistsException(errorMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to give to the constructor the file names to allow getFile()
to be used by the caller. C.f. doc
+ dumpFile.toString() | ||
+ " could not be processed since file " | ||
+ e.getFile() | ||
+ " already exists. Try deleting the file or dumpfile directory to attempt a new download."); | ||
+ " already exists. Try deleting the file or dumpfile directory to attempt a new download."; | ||
logger.error(errorMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the logging is still useful now that we raise the exception again.
Those are the changes proposed by @brett-matson in #523, minus the unrelated commits.