Address main scalastyle errors - #196 #248

greebie · 2018-07-31T22:32:33Z

GitHub issue(s):

#196 , also see #212 in discussion below

What does this Pull Request do?

Provides main fixes for scalastyle where the changes do not require major code refactoring.

all Magic Numbers provide vals
cases of duplicate "strings" (usually in test) provided with vals
public methods have explicit Types provided
general linting errors (missing spaces etc.)
tried to resolve underscore.import errors, or ignore them where it makes sense to (e.g. main aut classes or sparksql.functions._

How should this be tested?

Mostly mvn scalastyle:check but it should also pass Travis and tests.

Additional Notes:

I have ignored any scalastyle error where I would have to refactor code to make it work.
"Avoid null" errors were the most common. I have created an issue in #212 about how to resolve these (if we indeed wish to). Looking at the code, I think it's possible that resolving these could help us with a few null Pointer errors. But I'd like to address them in a different PR, because it involves different approaches to some conditionals and may require documentation changes.

Interested parties

@ruebot

Fix some magic numbers & duplicate string runs.

…ire refactoring.

codecov · 2018-07-31T22:49:03Z

Codecov Report

Merging #248 into master will increase coverage by 0.37%.
The diff coverage is 73.62%.

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   70.57%   70.94%   +0.37%     
==========================================
  Files          41       41              
  Lines         982     1005      +23     
  Branches      179      180       +1     
==========================================
+ Hits          693      713      +20     
- Misses        232      235       +3     
  Partials       57       57

Impacted Files	Coverage Δ
...scala/io/archivesunleashed/app/ExtractGraphX.scala	`92.1% <ø> (ø)`	⬆️
...io/archivesunleashed/matchbox/TupleFormatter.scala	`57.14% <ø> (ø)`	⬆️
...ala/io/archivesunleashed/matchbox/ComputeMD5.scala	`100% <ø> (ø)`	⬆️
src/main/scala/io/archivesunleashed/package.scala	`84.11% <ø> (ø)`	⬆️
...scala/io/archivesunleashed/app/WriteGraphXML.scala	`100% <ø> (ø)`	⬆️
.../io/archivesunleashed/app/PlainTextExtractor.scala	`100% <ø> (ø)`	⬆️
...ala/io/archivesunleashed/matchbox/RemoveHTML.scala	`100% <ø> (ø)`	⬆️
.../scala/io/archivesunleashed/app/ExtractGraph.scala	`0% <ø> (ø)`	⬆️
...c/main/scala/io/archivesunleashed/df/package.scala	`90.47% <ø> (ø)`	⬆️
...o/archivesunleashed/app/ExtractPopularImages.scala	`100% <ø> (ø)`	⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afe9254...af65a99. Read the comment docs.

ruebot

Nice work.

Couple fixes need to happen, as well as some questions answered.

ruebot · 2018-07-31T22:51:53Z

src/main/scala/io/archivesunleashed/app/CommandLineApp.scala

      throw new IllegalArgumentException()
-    case other => throw other
+    case other: Any => throw other
+


Do we need this blank line?

ruebot · 2018-07-31T22:52:11Z

src/main/scala/io/archivesunleashed/app/ExtractEntities.scala

 import org.apache.spark.SparkContext
 import org.apache.spark.rdd.RDD

+


Do we need this blank line?

ruebot · 2018-07-31T22:53:39Z

src/main/scala/io/archivesunleashed/app/ExtractPopularImages.scala

@@ -45,7 +47,6 @@ object ExtractPopularImages {
      .reduceByKey((image1, image2) => (image1._1, image1._2, image1._3 + image2._3))
      .map(x=> (x._2._3, x._2._2))
      .takeOrdered(limit)(Ordering[Int].on(x => -x._1))
-    res.foreach(x => println(x._1 + "\t" + x._2))


Why is this being removed? Is it just printing to a stdout?

Yeah - it was just printing.
Scalastyle rejects all println.
I can restore it and use a // scalastyle:off if it's necessary, however.

I wonder if @ianmilligan1 can speak to usefulness of a popular images printout here if we already have the RDD of images available (i.e. they can be mapped through and printed out in scripts if needed)?

I just ran it on both master and your branch, @greebie, and yes - probably that println was in there for debugging. The saveAsTextFile output is identical to the printed lines, so we were getting them twice. I think this is a good alteration.

ruebot · 2018-07-31T22:56:31Z

src/main/scala/io/archivesunleashed/matchbox/NER3Classifier.scala

@@ -26,7 +26,7 @@ import java.util
 import scala.collection.mutable


You need to change the name of this file as well, if you're changing it to NERClassifier. Make sure you do a git mv foo bar, otherwise you'll lose the history on the file.

Thanks for the fix. @ianmilligan1 I think this should be tested in case I broke something. There are no working unit tests for this I do not think.

Tested and works well (updated the docs which were outdated, actually).

ruebot · 2018-07-31T22:58:19Z

src/main/scala/io/archivesunleashed/matchbox/ExtractImageDetails.scala

@@ -22,7 +22,7 @@ import java.nio.charset.StandardCharsets
 import org.apache.commons.codec.binary.Hex


-/** Information about an image. e.g. width, height*/
+/** Information about an image. e.g. width, height */


Put a full stop after height.

ruebot · 2018-07-31T23:00:48Z

src/main/scala/io/archivesunleashed/util/TweetUtils.scala

@@ -26,6 +28,7 @@ object TweetUtils {
    * @param tweet JValue/JSON object containing Twitter API data (JSON)
    */
  implicit class JsonTweet(tweet: JValue) {
+    val userBranch = "user"


Can you speak to why userBranch?

userBranch refers to user as a branch of tweet json. The main error that wants to be fixed here is multiple occurrences of "user." Next commit will just use user as I agree that's less confusing.

ruebot · 2018-07-31T23:03:15Z

src/test/scala/io/archivesunleashed/matchbox/ExtractBoilerPipeTextTest.scala

@@ -29,10 +29,10 @@ class ExtractBoilerPipeTextTest extends FunSuite {
   <footer>Copyright 2017</footer>"""
  var boiler = """Copyright 2017"""

-  test("Collects boilerpip") {
+  test("Collects boilerpiep") {


ianmilligan1 · 2018-07-31T23:46:02Z

Ok, let me test NER and look into images - should be able to do so tomorrow!

greebie added 5 commits July 30, 2018 12:35

Deal with wildcard import lint issues.

9d18257

Fix some magic numbers & duplicate string runs.

More lint fixes, mostly explicit import warnings.

2e0b831

More lint fixes.

dab0316

Got scalastyle errors down to 75.

8b4c753

Final commit for scalastyle for now. All other scalastyle issues requ…

9ab7c6f

…ire refactoring.

ruebot requested changes Jul 31, 2018

View reviewed changes

Code review.

af65a99

ianmilligan1 approved these changes Aug 1, 2018

View reviewed changes

ruebot approved these changes Aug 1, 2018

View reviewed changes

ruebot merged commit 77dbd51 into master Aug 1, 2018

ruebot deleted the issue-196 branch August 1, 2018 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address main scalastyle errors - #196 #248

Address main scalastyle errors - #196 #248

greebie commented Jul 31, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

ruebot left a comment

ruebot Jul 31, 2018

ruebot Jul 31, 2018

ruebot Jul 31, 2018

greebie Jul 31, 2018 •

edited

Loading

greebie Jul 31, 2018

ianmilligan1 Aug 1, 2018

ruebot Jul 31, 2018 •

edited

Loading

greebie Jul 31, 2018

ianmilligan1 Aug 1, 2018

ruebot Jul 31, 2018

ruebot Jul 31, 2018

greebie Jul 31, 2018

ruebot Jul 31, 2018

ianmilligan1 commented Jul 31, 2018

		import org.apache.spark.SparkContext
		import org.apache.spark.rdd.RDD

		@@ -26,7 +26,7 @@ import java.util
		import scala.collection.mutable

Address main scalastyle errors - #196 #248

Address main scalastyle errors - #196 #248

Conversation

greebie commented Jul 31, 2018

What does this Pull Request do?

How should this be tested?

Additional Notes:

Interested parties

codecov bot commented Jul 31, 2018 • edited Loading

Codecov Report

ruebot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greebie Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruebot Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianmilligan1 commented Jul 31, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

greebie Jul 31, 2018 •

edited

Loading

ruebot Jul 31, 2018 •

edited

Loading