Add spotbugs and correct found problems #587
Conversation
|
Hi @tamird, thanks for working on this stuff! So I assume you ran all the tests locally and they passed, but did you also run the GUI locally and try loading up some test files and tabulating them? I ask because you did some things that I believe might break the GUI (i.e. changing classes from |
HEdingfield
requested review from
HEdingfield,
moldover and
tarheel
and removed request for
HEdingfield and
moldover
|
Right you are; reverted some of those changes (and added doc comments instead to appease the style checker). |
tamird
force-pushed
the
annotations
branch
3 times, most recently
from
b62634d
to
11cad79
Compare
tamird
force-pushed
the
annotations
branch
2 times, most recently
from
2ae14d1
to
6cc0577
Compare
|
Removed the warning suppressions as well; could not get that to produce a failure locally. Will revert some/all if CI is unhappy. |
|
@tamird not exactly sure how you're force pushing all these changes, but would you mind submitting a commit for each round of changes instead? We always squash before the merge anyway, and it would make things easier to review. |
|
May I suggest an alternative review tool which deals with multi-commit PRs more sanely? https://reviewable.io/ does a great job of this without forcing a workflow that produces a messy git history and without forcing single-commit PRs. This PR intentionally contains multiple commits, each of which is semantically separate, so I would prefer this not to be squashed on merge. This PR can be reviewed here. |
This file contains local configuration generated by Intellij; it is already included in .gitignore.
Remove Xlint:unchecked and Xlint:deprecation; it is unclear why these were present. Remove always-clean configuration; relying on clean for build correctness is an anti-pattern.
This also checks style. Update to checkstyle 10.2 while I'm here and clean up all existing style violations (mainly missing docs comments on public classes).
> Task :spotbugsTest M D NP: Possible null pointer dereference in network.brightspots.rcv.TabulatorTests.runTabulationTest(String) due to return value of called method Dereferenced at TabulatorTests.java:[line 152] M B OS: network.brightspots.rcv.TabulatorTests.fileCompare(String, String) may fail to close stream At TabulatorTests.java:[line 58] H I Dm: Found reliance on default encoding in network.brightspots.rcv.TabulatorTests.fileCompare(String, String): new java.io.FileReader(String) At TabulatorTests.java:[line 57] > Task :spotbugsTest FAILED > Task :spotbugsMain M X OBL: network.brightspots.rcv.JsonParser.readFromFile(String, Class, boolean) may fail to clean up java.io.Reader Obligation to clean up resource created at JsonParser.java:[line 42] is not discharged H I Dm: Found reliance on default encoding in network.brightspots.rcv.JsonParser.readFromFile(String, Class, boolean): new java.io.FileReader(String) At JsonParser.java:[line 42] M P WMI: network.brightspots.rcv.ResultsWriter.getCandidatesWithRanksList(Map) makes inefficient use of keySet iterator instead of entrySet iterator At ResultsWriter.java:[line 168] M P WMI: network.brightspots.rcv.ResultsWriter.setCandidatesToRoundEliminated(Map) makes inefficient use of keySet iterator instead of entrySet iterator At ResultsWriter.java:[line 245] M P WMI: network.brightspots.rcv.ResultsWriter.setWinnerToRound(Map) makes inefficient use of keySet iterator instead of entrySet iterator At ResultsWriter.java:[line 256] M P WMI: network.brightspots.rcv.ResultsWriter.generatePrecinctSummaryFiles(Map, Map, Map) makes inefficient use of keySet iterator instead of entrySet iterator At ResultsWriter.java:[line 289] M P WMI: network.brightspots.rcv.ResultsWriter.updateCandidateNamesInTally(Map) makes inefficient use of keySet iterator instead of entrySet iterator At ResultsWriter.java:[line 880] M P WMI: network.brightspots.rcv.ResultsWriter.addActionObjects(String, List, int, ArrayList, TallyTransfers) makes inefficient use of keySet iterator instead of entrySet iterator At ResultsWriter.java:[line 921] M P WMI: network.brightspots.rcv.ClearBallotCvrReader.readCastVoteRecords(List, String) makes inefficient use of keySet iterator instead of entrySet iterator At ClearBallotCvrReader.java:[line 109] H I Dm: Found reliance on default encoding in network.brightspots.rcv.ClearBallotCvrReader.readCastVoteRecords(List, String): new java.io.FileReader(String) At ClearBallotCvrReader.java:[line 51] M D REC: Exception is caught when Exception is not thrown in network.brightspots.rcv.HartCvrReader.readCastVoteRecord(List, Path) At HartCvrReader.java:[line 128] M X OBL: network.brightspots.rcv.HartCvrReader.readCastVoteRecord(List, Path) may fail to clean up java.io.InputStream Obligation to clean up resource created at HartCvrReader.java:[line 80] is not discharged M X OBL: network.brightspots.rcv.CommonDataFormatReader.parseXml(List) may fail to clean up java.io.InputStream Obligation to clean up resource created at CommonDataFormatReader.java:[line 132] is not discharged M P WMI: network.brightspots.rcv.Tabulator.updatePastWinnerTallies() makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 319] M P WMI: network.brightspots.rcv.Tabulator.updatePastWinnerTallies() makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 273] M P WMI: network.brightspots.rcv.Tabulator.updatePastWinnerTallies() makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 297] M P WMI: network.brightspots.rcv.Tabulator.identifyWinners(Map, SortedMap) makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 481] M P WMI: network.brightspots.rcv.Tabulator.dropCandidatesBelowThreshold(SortedMap) makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 567] M P WMI: network.brightspots.rcv.Tabulator.runBatchElimination(SortedMap) makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 740] M P WMI: network.brightspots.rcv.Tabulator.computeTalliesForRound(int) makes inefficient use of keySet iterator instead of entrySet iterator At Tabulator.java:[line 1006] M C RCN: Nullcheck of config at line 143 of value previously dereferenced in network.brightspots.rcv.TabulatorSession.tabulate() At TabulatorSession.java:[line 143] H I Dm: Found reliance on default encoding in network.brightspots.rcv.TabulatorSession.tabulate(): new java.io.FileReader(String) At TabulatorSession.java:[line 152] M B OS: network.brightspots.rcv.GuiConfigController.loadTxtFileIntoString(String) may fail to close stream At GuiConfigController.java:[line 272] H I Dm: Found reliance on default encoding in network.brightspots.rcv.GuiConfigController.loadTxtFileIntoString(String): new java.io.InputStreamReader(InputStream) At GuiConfigController.java:[line 275] H I Dm: Found reliance on default encoding in network.brightspots.rcv.Tiebreak.doInteractiveCli(List): new java.util.Scanner(InputStream) At Tiebreak.java:[line 169] > Task :spotbugsMain FAILED
Totally open to new workflows! Anything to make things more like Critique would be great IMO. Maybe you can give us a quick walkthrough of reviewable.io sometime? Though, this may sound dumb, but the lack of dark mode makes it really hard for me to use as-is. Still interested in learning more about it though. |
|
@tamird I didn't do the squash and merge per your request; I did a merge commit. If you prefer it done with a rebase merge, let me know and we can revert. Not used to doing it this way, so not sure what the best practice is. |
|
Merge commit is the right thing to do! Thanks.
Happy to!
Hadn't considered this, looks like it's a known issue. |
|
@tamird just wanted to say thanks for this work and the suggestions. I'm curious about reviewable.io as well. Looks like they offer a free version for open repos which is nifty. |
See individual commits.