Deal with final scalastyle assessments: Issue 212 #249
Conversation
Remove try-catch from ExtractLinks as over kill. IOErrors should be caught in ArchiveRecordImpl (imho).
Codecov Report
@@ Coverage Diff @@
## master #249 +/- ##
==========================================
- Coverage 70.94% 70.11% -0.83%
==========================================
Files 41 41
Lines 1005 1024 +19
Branches 180 191 +11
==========================================
+ Hits 713 718 +5
- Misses 235 240 +5
- Partials 57 66 +9
Continue to review full report at Codecov.
|
| } | ||
| } | ||
|
|
||
| /** Extracts boilerplate. |
There was a problem hiding this comment.
Why is this function being removed?
There was a problem hiding this comment.
I believe the code on the right does the exact same job as the code on the left. I do not think the exception is needed. If it is, I think it would be better to catch the more specific issue coming from Exception and handle it accordingly with more detailed warnings. But I have not been able to think of a case, barring something really crazy hitting the server.
There was a problem hiding this comment.
I believe the code on the right does the exact same job as the code on the left?
Are you talking about a diff? Otherwise I don't gather what you're referring to here with left and right.
There was a problem hiding this comment.
Yeah - sorry. In the view I was using the changed code was on the right and the old code was on the left.
There was a problem hiding this comment.
More clearly, the extract method is incorporated into apply with no (known) differences in the way ExtractBoilerplateText works.
| } | ||
| } | ||
|
|
||
| /** Extracts boilerplate. |
|
Cool. I'll get this squashed and merged later on this evening then. |
GitHub issue(s):
#212
What does this Pull Request do?
It finishes off the scalastyle warnings by dealing with a number of
nullcalls which scalastyle does not like.Provides the final revisions to scalastyle problems, sometimes resulting in code refactoring.
In some cases, I felt like the Option[t] approach simplified the code and so I refactored (examples include ExtractDomain and ExtractLinks). These also had other style errors like using
return.I tried not to go overboard doing this, however.
In other cases, the only thing needed was to return an empty string "" instead of null.
In some cases, there seemed like no point in worrying about null. In this case, I used
// scalastyle:off nullExample is the NER stuff.For tests, the nulls were kept as a way to break things. I used
scalastyle:off nullthere as well.Overall, whether we like the Option[t] approach or not, I think having the scalastyle stuff formalized and clean will be good for future. We can also just turn off the "null" setting in the configuration.
How should this be tested?
Because this involved some code refactoring of pretty major functions (ExtractDomain e.g.) I would suggest testing it with a messy set of warcs just to make sure I did not break things. For the most part, other than switching "nulls" to empty strings, all of the current tests are the same and passed.
For discussion, is the idea of outputting a default bad output string: something like "aut_empty" that could be set globally. However, for now I just went with empty string.
Additional Notes:
I made it so that using the Option[t] did not affect example scripts. In some ways it would be better for the functions to send out Option[t]s but it would mean that people would have to learn scala in more detail which is something we are pulling away from.
Interested parties
@ianmilligan1 @lintool (because of style issues raised) @ruebot
Thanks in advance for your help with the Archives Unleashed Toolkit!