Add a message error to scalafixFileEvaluation #1251

mlachkar · 2020-10-05T10:20:45Z

Modify isSuccessful of ScalafixEvaluation: If a lintError is found, isSuccessful is false.
A LintError is produced when scalafix has run successfully and there are diagnostics with severity Error.
The issue is the default severity is Error, and rules don't usually modify that.
I think it's misleading to report that scalafix has failed, when there are diagnostics.
Fix the reporting of StaleSemanticDB error. We were reporting unexpected Error instead of StaleSemanticdbError

bjaglin

Makes sense! A few comments regarding the API changes.

bjaglin · 2020-10-05T11:52:19Z

scalafix-cli/src/main/scala/scalafix/internal/v1/MainOps.scala

@@ -104,6 +104,7 @@ object MainOps {
        val files = getFilesFrom(args)
        val fileEvaluations = {
          files.map { file =>
+            println(s"file = ${file}")


Suggested change

println(s"file = ${file}")

bjaglin · 2020-10-05T11:53:47Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixFileEvaluation.java

@@ -22,6 +22,8 @@
     */
    boolean isSuccessful();

+    Optional<String> getMessageError();


could you add some javadoc? does "error" include lint errors?

ok. It includes lintErrors.

sorry if I wasn't clear but I was talking about getMessageError - I think the getErrors are clear enough given their signature so the javadoc there is redundant.

Actually this message error won't be empty except if the error is

case _: ParseException => case _: SemanticDocument.Error.MissingSemanticdb => case _: StaleSemanticDB => case NonFatal(_) =>

and messageError do not include lint errors. LinterError are described in diagnostics

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEvaluation.java

tgodzik

Looks good! Just one question: is scalafix-interfaces/.DS_Store added on purpose? Not sure what that file does.

bjaglin · 2020-10-05T20:45:57Z

scalafix-tests/unit/src/test/scala/scalafix/tests/interfaces/ScalafixArgumentsSuite.scala

+    val evaluation2 = args.evaluate()
+    assert(evaluation2.getErrors.head.toString == "StaleSemanticdbError")
+    assert(evaluation2.getMessageError == Optional.empty())
+    assert(!evaluation2.isSuccessful)
+    val fileEval = evaluation2.getFileEvaluations.head
+    assert(fileEval.getErrors.head.toString == "StaleSemanticdbError")
+    assert(fileEval.getMessageError.get.startsWith("Stale SemanticDB"))


👍 can we cover the other isSuccessful while we are at it?

Suggested change

val evaluation2 = args.evaluate()

assert(evaluation2.getErrors.head.toString == "StaleSemanticdbError")

assert(evaluation2.getMessageError == Optional.empty())

assert(!evaluation2.isSuccessful)

val fileEval = evaluation2.getFileEvaluations.head

assert(fileEval.getErrors.head.toString == "StaleSemanticdbError")

assert(fileEval.getMessageError.get.startsWith("Stale SemanticDB"))

val evaluation2 = args.evaluate()

assert(evaluation2.getErrors.head.toString == "StaleSemanticdbError")

assert(evaluation2.getMessageError == Optional.empty())

assert(!evaluation2.isSuccessful)

val fileEval = evaluation2.getFileEvaluations.head

assert(fileEval.getErrors.head.toString == "StaleSemanticdbError")

assert(fileEval.getMessageError.get.startsWith("Stale SemanticDB"))

assert(!fileEval.isSuccessful)

bjaglin · 2020-10-05T20:47:16Z

scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixEvaluationImpl.scala

  override def getErrors: Array[ScalafixError] = {
    ScalafixErrorImpl.fromScala(error)
  }
+  override def isSuccessful: Boolean =
+    getErrors.toList.filter(_ != ScalafixError.LinterError).isEmpty


i think the behavior change of both isSuccessful is lacking a test case to prevent regressions

ok, good idea

mlachkar · 2020-10-07T16:39:37Z

LinterErrors were only added at the end, when scalafixFileEvaluations contain nonEmpty diagnostics (through adjustExitCode).
ScalafixEvaluation is a new api, that is not meant to be used as a cli, and only thought code calls. LinterErrors are already represented as a ScalafixDiagnostics.
The last commit removes the adjustExitCode, so we don't need anymore to filter LinterErrors from errors, when verifying isSuccessful.

I did a choice to concatenate all scalafixFileEval error messages if scalafix has run successfully only on a number of files. Just a suggestion

bjaglin

I am afraid I am still having a hard time figuring out the behavior by looking at the docs. I think the problem comes from the re-use of the enum ScalafixError.

This API is brand new, maybe it's still time to deprecate getErrorMessage (which concatenates errors in a way that makes it hard to consume for a non-human) & getErrors which have a similar signature in ScalafixEvaluation and ScalafixFileEvaluation, yet different possible values.

What about replacing them with ScalafixEvaluationError[] getEvaluationErrors(), ScalafixEvaluationError being an interface with a getType returning an enum with only "operational errors" (maybe only the ones specific to the whole invocation, no need to aggregate the ones from files?), and getMessage? Same thing for ScalafixFileEvaluationError, with its own embedded enum. It sounds like a lot of changes, but it would avoid a lot of ambiguous documentation and testing.

bjaglin · 2020-10-07T19:06:22Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEvaluation.java

+     * @return a more detailed error message that can be
+     *  - an explanation of why scalafix failed running at all
+     *  - a concatenation of all {@Link ScalafixFileEvaluation} message Errors


Cosmetic, but I think you need tags for this list of bullet points to appear properly in Javadoc like https://javadoc.io/doc/ch.epfl.scala/scalafix-interfaces/0.9.21/scalafix/interfaces/ScalafixFileEvaluation.html. Have a look at the rest of the Javadoc, it's very clean, so I think we should try to keep the same standard.

bjaglin · 2020-10-07T19:17:02Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixFileEvaluation.java

+     * @return a more detailed error in case of scalafix ParseError, StaleSemanticdbError,
+     * MissingSemanticdbError or an UnexpectedError. LinterErrors are found in {@link ScalafixDiagnostic}


I am not in front of an IDE, but looking at the code, it looks like the error message is only defined for UnexpectedError.

scalafix/scalafix-cli/src/main/scala/scalafix/internal/v1/MainOps.scala

Lines 122 to 125 in e31e5c9

case Failure(exception) =>

ScalafixFileEvaluationImpl

.from(file, ExitStatus.UnexpectedError, exception.getMessage)(

args

true, but this has been fixed in this PR.

case Failure(exception) => .from(file, ExitStatus.from(exception), exception.getMessage) ... def from(throwable: Throwable): (ExitStatus) = throwable match { case _: ParseException => ExitStatus.ParseError case _: SemanticDocument.Error.MissingSemanticdb => ExitStatus.MissingSemanticdbError case _: StaleSemanticDB => ExitStatus.StaleSemanticdbError case NonFatal(_) => ExitStatus.UnexpectedError }

bjaglin · 2020-10-07T19:36:14Z

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEvaluation.java

     */
    boolean isSuccessful();

    ScalafixError[] getErrors();

+    /**
+     *
+     * @return a more detailed error message that can be


more than what?

the error , wchich is only the type of the Enum

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEvaluation.java

scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixFileEvaluationImpl.scala

mlachkar · 2020-10-08T08:01:16Z

I agree. It will be easier to add new ScalafixErrors. I will rework this. Thanks
The getErrorMessage doesn't need to be a concatenation. it can be just error message in case of scalafix not running at all.
And we can keep errror message of file, in ScalafixFileEvaluation part

bjaglin

Much clearer with new enums! A last round of comments about the behavior and a clearer javadoc, and I guess we are good to go!

.scalafix.conf

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixFileEvaluation.java

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixEvaluation.java

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixFileEvaluation.java

scalafix-interfaces/src/main/java/scalafix/interfaces/EvaluationError.java

bjaglin · 2020-10-27T07:42:02Z

(rebase needed)

* Modify isSuccessful of ScalafixEvaluation: If a lintError is found, isSuccessful is false. A LintError is produced when scalafix has run successfully and there are diagnostics with severity Error. The issue is the default severity is Error, and rules don't usually modify that. I think it's misleading to report that scalafix has failed, when there are diagnostics. * Fix the reporting of StaleSemanticDB error. We were reporting unexpected Error instead of StaleSemanticdbError

* fix javadoc * keep the same behavior for isSuccesful for scalafixFileEvaluation

…calafixEvaluation - Deprecate old methods to retrieve errors in ScalafixEvaluation and ScalafixFileEvaluation

mlachkar · 2020-10-27T08:17:19Z

Thanks for your review. I addressed all your comments.

bjaglin

👍

mlachkar linked an issue Oct 5, 2020 that may be closed by this pull request

Error message not exposed in the API #1250

Closed

mlachkar requested review from bjaglin and tgodzik October 5, 2020 11:00

bjaglin reviewed Oct 5, 2020

View reviewed changes

mlachkar force-pushed the errors branch 2 times, most recently from e1edab8 to 837652b Compare October 5, 2020 13:11

tgodzik reviewed Oct 5, 2020

View reviewed changes

mlachkar force-pushed the errors branch from 837652b to 67d5c84 Compare October 5, 2020 16:04

bjaglin reviewed Oct 5, 2020

View reviewed changes

mlachkar force-pushed the errors branch 3 times, most recently from 1522445 to c80fcb9 Compare October 7, 2020 16:31

mlachkar force-pushed the errors branch from c80fcb9 to 51de8a3 Compare October 7, 2020 16:42

mlachkar requested a review from bjaglin October 7, 2020 16:44

bjaglin reviewed Oct 7, 2020

View reviewed changes

mlachkar requested a review from bjaglin October 26, 2020 15:27

bjaglin reviewed Oct 27, 2020

View reviewed changes

mlachkar added 4 commits October 27, 2020 09:07

Handle more exception case, as it's done in scalafix-cli

fa74105

* fix javadoc * keep the same behavior for isSuccesful for scalafixFileEvaluation

Add test cases for isSuccessful

a1d9e48

Add FileEvaluationError and EvaluationError to represent errors for S…

d1ed83f

…calafixEvaluation - Deprecate old methods to retrieve errors in ScalafixEvaluation and ScalafixFileEvaluation

mlachkar force-pushed the errors branch from cc9de53 to 9bb6f29 Compare October 27, 2020 08:12

Various fixes

0e8372c

mlachkar force-pushed the errors branch from 9bb6f29 to 0e8372c Compare October 27, 2020 08:14

mlachkar requested a review from bjaglin October 27, 2020 08:17

bjaglin approved these changes Oct 27, 2020

View reviewed changes

mlachkar merged commit 1ea958e into scalacenter:master Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a message error to scalafixFileEvaluation #1251

Add a message error to scalafixFileEvaluation #1251

mlachkar commented Oct 5, 2020

bjaglin left a comment

bjaglin Oct 5, 2020

bjaglin Oct 5, 2020

mlachkar Oct 5, 2020

bjaglin Oct 5, 2020

mlachkar Oct 7, 2020

mlachkar Oct 7, 2020

tgodzik left a comment

bjaglin Oct 5, 2020

bjaglin Oct 5, 2020

mlachkar Oct 6, 2020

mlachkar commented Oct 7, 2020 •

edited

Loading

bjaglin left a comment •

edited

Loading

bjaglin Oct 7, 2020

bjaglin Oct 7, 2020

mlachkar Oct 8, 2020

bjaglin Oct 7, 2020

mlachkar Oct 8, 2020

mlachkar commented Oct 8, 2020 •

edited

Loading

bjaglin left a comment

bjaglin commented Oct 27, 2020

mlachkar commented Oct 27, 2020

bjaglin left a comment

		* @return a more detailed error in case of scalafix ParseError, StaleSemanticdbError,
		* MissingSemanticdbError or an UnexpectedError. LinterErrors are found in {@link ScalafixDiagnostic}

	case Failure(exception) =>
	ScalafixFileEvaluationImpl
	.from(file, ExitStatus.UnexpectedError, exception.getMessage)(
	args

Add a message error to scalafixFileEvaluation #1251

Add a message error to scalafixFileEvaluation #1251

Conversation

mlachkar commented Oct 5, 2020

bjaglin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik 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

mlachkar commented Oct 7, 2020 • edited Loading

bjaglin left a comment • 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

mlachkar commented Oct 8, 2020 • edited Loading

bjaglin left a comment

Choose a reason for hiding this comment

bjaglin commented Oct 27, 2020

mlachkar commented Oct 27, 2020

bjaglin left a comment

Choose a reason for hiding this comment

mlachkar commented Oct 7, 2020 •

edited

Loading

bjaglin left a comment •

edited

Loading

mlachkar commented Oct 8, 2020 •

edited

Loading