Skip to content

Commit

Permalink
Enforcing proper formatting of descriptions and explanations (#331)
Browse files Browse the repository at this point in the history
* UT for non-empty description ending in period

* Irrelevant auto-formatting

* UT for description and explanation

* Unifying spelling of code in descriptions and explanations.

* Wrapping code samples in description and explanation into ``.
  • Loading branch information
mccartney authored Mar 29, 2020
1 parent e0bf324 commit e947418
Show file tree
Hide file tree
Showing 22 changed files with 91 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@ class LonelySealedTrait
}

private def inspectParents(parents: List[Tree]): Unit = {
parents.foreach {
parent =>
for (c <- parent.tpe.baseClasses)
implementedClasses.add(c.name.toString)
parents.foreach { parent =>
for (c <- parent.tpe.baseClasses)
implementedClasses.add(c.name.toString)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ class ComparisonToEmptyList
extends Inspection(
text = "Comparison to empty list",
defaultLevel = Levels.Info,
description = "Checks for code like a == List() or a == Nil.",
explanation = "Prefer use of isEmpty instead of comparison to an empty List."
description = "Checks for code like `a == List()` or `a == Nil`.",
explanation = "Prefer use of `isEmpty` instead of comparison to an empty List."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ class ComparisonToEmptySet
extends Inspection(
text = "Comparison to empty set",
defaultLevel = Levels.Info,
description = "Checks for code like a == Set() or a == Set.empty.",
explanation = "Prefer use of isEmpty instead of comparison to an empty Set."
description = "Checks for code like `a == Set()` or `a == Set.empty`.",
explanation = "Prefer use of `isEmpty` instead of comparison to an empty Set."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ class ExistsSimplifiableToContains
extends Inspection(
text = "Exists simplifiable to contains",
defaultLevel = Levels.Info,
description = "Checks if exists() can be simplified to contains().",
explanation = "exists(x => x == y) can be replaced with contains(y)."
description = "Checks if `exists()` can be simplified to `contains()`.",
explanation = "`exists(x => x == y)` can be replaced with `contains(y)`."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class FilterDotHead
defaultLevel = Levels.Info,
description = "Checks for use of filter().head.",
explanation =
"filter().head can throw an exception if the collection is empty - it can be replaced with find() match {...}."
"`filter().head` can throw an exception if the collection is empty - it can be replaced with `find() match {...}`."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ class FilterDotHeadOption
defaultLevel = Levels.Info,
description = "Checks for use of filter().headOption.",
explanation =
"filter() scans the entire collection, which is unnecessary if you only want to get the first element that satisfies the predicate - filter().headOption can be replaced with find() to potentially avoid scanning the entire collection."
"`filter()` scans the entire collection, which is unnecessary if you only want to get the first element that " +
"satisfies the predicate - `filter().headOption` can be replaced with `find()` to potentially avoid scanning the entire collection."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ class FilterDotIsEmpty
defaultLevel = Levels.Info,
description = "Checks for use of filter().isEmpty.",
explanation =
"filter() scans the entire collection, which can potentially be avoided if the element exists in the collection - filter().isEmpty can be replaced with !exists()."
"`filter()` scans the entire collection, which can potentially be avoided if the element exists in the " +
"collection - `filter().isEmpty` can be replaced with `!exists()`."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class FilterDotSize
text = "filter().size() instead of count()",
defaultLevel = Levels.Info,
description = "Checks if filter().size can be simplified to count().",
explanation = "filter().size can be replaced with count(), which is more concise."
explanation = "`filter().size` can be replaced with `count()`, which is more concise."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class FilterOptionAndGet
text = "filter(_.isDefined).map(_.get) instead of flatten",
defaultLevel = Levels.Info,
description = "Checks whether the expression can be rewritten using flatten.",
explanation = "filter(_.isDefined).map(_.get) can be replaced with flatten."
explanation = "`filter(_.isDefined).map(_.get)` can be replaced with `flatten`."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ class FindAndNotEqualsNoneReplaceWithExists
extends Inspection(
text = "find(x => ) != None instead of exists(x =>)",
defaultLevel = Levels.Info,
description = "Checks whether find() can be replaced with exists().",
explanation = "find() != None can be replaced with exists(), which is more concise."
description = "Checks whether `find()` can be replaced with exists().",
explanation = "`find() != None` can be replaced with `exists()`, which is more concise."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ class FindDotIsDefined
extends Inspection(
text = "find().isDefined() instead of exists()",
defaultLevel = Levels.Info,
description = "Checks whether find() can be replaced with exists().",
explanation = "find().isDefined can be replaced with exists(), which is more concise."
description = "Checks whether `find()` can be replaced with `exists()`.",
explanation = "`find().isDefined` can be replaced with `exists()`, which is more concise."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ class ReverseFunc
defaultLevel = Levels.Info,
description = "Checks for use of reverse followed by head/headOption/iterator/map.",
explanation =
"reverse followed by head, headOption, iterator, or map can be replaced, respectively, with last, lastOption, reverseIterator, or reverseMap."
"`reverse` followed by `head`, `headOption`, `iterator`, or `map` can be replaced, respectively, with " +
"`last`, `lastOption`, `reverseIterator`, or `reverseMap`."
) {

object FuncReplace {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class ReverseTailReverse
text = "reverse.tail.reverse instead of init",
defaultLevel = Levels.Info,
description = "Checks for use of reverse.tail.reverse.",
explanation = "reverse.tail.reverse can be replaced with init, which is more concise."
explanation = "`reverse.tail.reverse` can be replaced with `init`, which is more concise."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class ReverseTakeReverse
text = "reverse.take().reverse instead of takeRight",
defaultLevel = Levels.Info,
description = "Checks for use of reverse.take().reverse.",
explanation = "reverse.take().reverse can be replaced with takeRight, which is more concise."
explanation = "`reverse.take().reverse` can be replaced with `takeRight`, which is more concise."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ class UnsafeContains
extends Inspection(
text = "Unsafe contains",
defaultLevel = Levels.Error,
description = "Checks Seq.contains() and Option.contains() for unrelated types.",
description = "Checks `Seq.contains()` and `Option.contains()` for unrelated types.",
explanation =
"contains() accepts arguments af any type, which means you might be checking if your collection contains an element of an unrelated type."
"`contains()` accepts arguments af any type, which means you might be checking if your collection " +
"contains an element of an unrelated type."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class RepeatedIfElseBody
private def twoBlocksStartWithTheSame(oneBlock: Block, another: Block): Boolean = {
(oneBlock.children.headOption, another.children.headOption) match {
case (Some(statement1), Some(statement2)) if statement1.toString == statement2.toString => true
case _ => false
case _ => false
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import com.sksamuel.scapegoat._
/** @author Stephen Samuel */
class BigDecimalScaleWithoutRoundingMode
extends Inspection(
text = "BigDecimal setScale() without rounding mode",
text = "BigDecimal `setScale()` without rounding mode",
defaultLevel = Levels.Warning,
description =
"Checks for use of setScale() on a BigDecimal without setting the rounding mode can throw an exception.",
"Checks for use of `setScale()` on a BigDecimal without setting the rounding mode can throw an exception.",
explanation =
"When using setScale() on a BigDecimal without setting the rounding mode, this can throw an exception if rounding is required. Did you mean to call setScale(s, RoundingMode.XYZ)?"
"When using `setScale()` on a BigDecimal without setting the rounding mode, this can throw an exception " +
"if rounding is required. Did you mean to call `setScale(s, RoundingMode.XYZ)`?"
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class BrokenOddness
defaultLevel = Levels.Warning,
description = "Checks for potentially broken odd checks.",
explanation =
"Code that attempts to check for oddness using x % 2 == 1 will fail on negative numbers. Consider using x % 2 != 0"
"Code that attempts to check for oddness using `x % 2 == 1` will fail on negative numbers. Consider using `x % 2 != 0`."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class FinalizerWithoutSuper
defaultLevel = Levels.Warning,
description = "Checks for overridden finalizers that do not call super.",
explanation =
"Finalizers should call super.finalize() to ensure superclasses are able to run their finalization logic."
"Finalizers should call `super.finalize()` to ensure superclasses are able to run their finalization logic."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
Expand Down
25 changes: 14 additions & 11 deletions src/main/scala/com/sksamuel/scapegoat/io/HtmlReportWriter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -93,31 +93,34 @@ object HtmlReportWriter {
</body>

def warnings(reporter: Feedback) = {
reporter.warningsWithMinimalLevel.map {
warning =>
val source = warning.sourceFileNormalized + ":" + warning.line
<div class="warning">
reporter.warningsWithMinimalLevel.map { warning =>
val source = warning.sourceFileNormalized + ":" + warning.line
<div class="warning">
<div class="source">
{source}
</div>
<div class="title">
{warning.level match {
case Levels.Info => <span class="label label-info">Info</span>
case Levels.Warning => <span class="label label-warning">Warning</span>
case Levels.Error => <span class="label label-danger">Error</span>
}}&nbsp;{warning.text}&nbsp; <span class="inspection">
{
warning.level match {
case Levels.Info => <span class="label label-info">Info</span>
case Levels.Warning => <span class="label label-warning">Warning</span>
case Levels.Error => <span class="label label-danger">Error</span>
}
}&nbsp;{warning.text}&nbsp; <span class="inspection">
{warning.inspection}
</span>
</div>
<div>
{warning.explanation}
</div>{warning.snippet match {
</div>{
warning.snippet match {
case None =>
case Some(snippet) =>
<div class="snippet">
{snippet}
</div>
}}
}
}
</div>
}
}
Expand Down
34 changes: 16 additions & 18 deletions src/main/scala/com/sksamuel/scapegoat/plugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -74,25 +74,23 @@ class ScapegoatPlugin(val global: Global) extends Plugin {
component.disableHTML = false
component.disableScalastyleXML = false
}
forProperty("overrideLevels:") foreach {
option =>
component.feedback.levelOverridesByInspectionSimpleName = option
.drop("overrideLevels:".length)
.split(":")
.map {
nameLevel =>
nameLevel.split("=") match {
case Array(insp, level) => insp -> Levels.fromName(level)
case _ =>
throw new IllegalArgumentException(
s"Malformed argument to 'overrideLevels': '$nameLevel'. " +
"Expecting 'name=level' where 'name' is the simple name of " +
"an inspection and 'level' is the simple name of a " +
"com.sksamuel.scapegoat.Level constant, e.g. 'Warning'."
)
}
forProperty("overrideLevels:") foreach { option =>
component.feedback.levelOverridesByInspectionSimpleName = option
.drop("overrideLevels:".length)
.split(":")
.map { nameLevel =>
nameLevel.split("=") match {
case Array(insp, level) => insp -> Levels.fromName(level)
case _ =>
throw new IllegalArgumentException(
s"Malformed argument to 'overrideLevels': '$nameLevel'. " +
"Expecting 'name=level' where 'name' is the simple name of " +
"an inspection and 'level' is the simple name of a " +
"com.sksamuel.scapegoat.Level constant, e.g. 'Warning'."
)
}
.toMap
}
.toMap
}
forProperty("sourcePrefix:") match {
case Some(option) =>
Expand Down
27 changes: 27 additions & 0 deletions src/test/scala/com/sksamuel/scapegoat/AllInspectionsTest.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.sksamuel.scapegoat

import org.scalatest.freespec.AnyFreeSpec
import org.scalatest.matchers.should.Matchers

class AllInspectionsTest extends AnyFreeSpec with Matchers {

val code = "\\s[A-Za-z0-9.]+\\(\\)\\s".r

ScapegoatConfig.inspections.foreach { insp =>
insp.getClass.getSimpleName - {
"should have a properly-formatted description" in {
insp.description.trim shouldNot be("")
insp.description.last should be('.')
insp.description.head.toUpper should be(insp.description.head)
code.findAllIn(insp.description).hasNext should be(false)
}

"should have a properly-formatted explanation" in {
insp.explanation.trim shouldNot be("")
insp.explanation.last should (be('.') or be('?'))
insp.explanation.head.toUpper should be(insp.explanation.head)
code.findAllIn(insp.explanation).hasNext should be(false)
}
}
}
}

0 comments on commit e947418

Please sign in to comment.