Add strict mode to fail the Gradle task in case of parsing errors #67

SimonMarquis · 2023-07-22T08:34:27Z

Closes #37

jamesonwilliams

LGTM

sort-dependencies-gradle-plugin/src/main/kotlin/com/squareup/sort/SortDependenciesTask.kt

autonomousapps

Thanks! Left some comments. While I'm not opposed to the user's potential desire to fail their build in the presence of parse errors, I don't think that is the correct default. Independently, I wouldn't make a behavior change like that outside of a major version change (but as I said, I'm also philosophically opposed to it).

In addition, could you add an entry to the README for this?

sort-dependencies-gradle-plugin/src/main/kotlin/com/squareup/sort/SortDependenciesTask.kt

sort-dependencies-gradle-plugin/src/test/groovy/com/squareup/sort/FunctionalSpec.groovy

autonomousapps

Something is not quite adding up 😅

autonomousapps · 2023-07-24T18:22:58Z

sort-dependencies-gradle-plugin/src/main/kotlin/com/squareup/sort/SortDependenciesTask.kt

@@ -50,6 +55,7 @@ abstract class SortDependenciesTask @Inject constructor(
    val buildScript = buildScript.get().asFile.absolutePath
    val mode = mode.getOrElse("sort")
    val verbose = verbose.getOrElse(false)
+    val strict = strict.getOrElse(false)


Should it be getOrElse() or isPresent()? Your docs suggest just passing --strict is all that is needed. If you want to get the value, then users would pass --strict=true or --strict=false (I think).

I'm pretty sure Gradle do that by default for Boolean properties, but let me double check. (I followed what was done for the verbose parameter)

Confirmed through code, and here is the corresponding docs:

Describes an option with the value true or false. Passing the option on the command line treats the value as true, for example --foo equates to true. The absence of the option uses the default value of the property. For each boolean option, an opposite option is created automatically. For example, --no-foo is created for the provided option --foo and --bar is created for --no-bar. Options whose name starts with --no are disable options and set the option value to false. An opposite option is only created if no option with the same name already exists for the task.

https://docs.gradle.org/8.2/userguide/custom_tasks.html#sec:supported_task_option_data_types

autonomousapps · 2023-07-24T18:23:29Z

sort-dependencies-gradle-plugin/src/test/groovy/com/squareup/sort/FunctionalSpec.groovy

@@ -81,7 +81,7 @@ final class FunctionalSpec extends Specification {
    Files.writeString(buildScript, BUILD_SCRIPT)

    when: 'We sort dependencies'
-    def result = buildAndFail(dir, 'checkSortDependencies', '--verbose')
+    def result = buildAndFail(dir, 'checkSortDependencies', '--verbose', '--strict')


Why is this test changing?

This test is changing because it is expected to fail: buildAndFail.

And the current exit code for this test is NOT_SORTED(3).
We must either:

use the --strict

or replace buildAndFail with build

(or both)

The fact that you had to change the test suggests the default behavior has changed, and I don't want the default behavior to change. New behavior behind a flag is fine, but I don't want to change current behavior.

SimonMarquis · 2023-07-25T18:15:49Z

Let's recap and make sure we are on the same page.
The current state of the main branch is:
If CLI returns non-zero exit code, the task fails (as I would expect).

This means any parsing error PARSE_ERROR(5) will fail the task, hence the use of buildAndFail in tests.

Therefore, if we want to configure this through a strict flag, we should instead have true as default value (and reflect the current behavior), and let users specify --no-strict (or --strict=false) to disable the default behavior.

That being said, if we go back to the main issue that lead to this PR, the output was the following:

Metrics:
  Not sorted:     0
  Already sorted: 1
  Parse errors:   1

...

BUILD SUCCESSFUL in 857ms

And here is the root issue! This should have been reported as a parsing error and the build should have failed.
Otherwise check mode would ignore parsing errors, while sort mode won't. And this makes not a lot of sense. Do you agree with that?

To be more precise, the root issue comes from this difference in behavior:

in sort mode:

private fun sort(/*...*/): Status {
  /*...*/
  return if (parseErrorCount == 0) SUCCESS else PARSE_ERROR
}

in check mode:

private fun check(/*...*/): Status {
  /*...*/
  val success = notSorted.isEmpty() /* 🐛 This looks like an error to me! */
  /*...*/
  return if (success) {
    SUCCESS
  } else if (parseErrorCount > 0) {
    PARSE_ERROR
  } else {
    NOT_SORTED
  }
}

What I propose is:

fixing the root issue, which is check mode not failing as it should in case of parsing error
(optional, if needed) offer an "ignore parsing error" flag instead.

Add strict mode to fail the Gradle task in case of parsing errors

c0e49ed

Closes #37

SimonMarquis mentioned this pull request Jul 22, 2023

Feature request: configurable option for whether or not parse errors fail the task #37

Open

jamesonwilliams approved these changes Jul 22, 2023

View reviewed changes

sort-dependencies-gradle-plugin/src/main/kotlin/com/squareup/sort/SortDependenciesTask.kt Outdated Show resolved Hide resolved

autonomousapps requested changes Jul 24, 2023

View reviewed changes

SimonMarquis and others added 5 commits July 24, 2023 09:41

Flip the default strict value back to false

bd9b5a3

Update the strict option description

e33f626

Request strict mode option while executing the test

863ae66

Configure JavaExecSpec to ignore exit code only in non-strict mode

34b82fa

Add entry in README.md

f7d7e01

autonomousapps reviewed Jul 24, 2023

View reviewed changes

Merge branch 'square:main' into issue-37

6e49839

SimonMarquis closed this by deleting the head repository Jan 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add strict mode to fail the Gradle task in case of parsing errors #67

Add strict mode to fail the Gradle task in case of parsing errors #67

SimonMarquis commented Jul 22, 2023

jamesonwilliams left a comment

autonomousapps left a comment

autonomousapps left a comment

autonomousapps Jul 24, 2023

SimonMarquis Jul 24, 2023

SimonMarquis Jul 24, 2023 •

edited

autonomousapps Jul 24, 2023

SimonMarquis Jul 24, 2023

SimonMarquis Jul 24, 2023

autonomousapps Jul 25, 2023

SimonMarquis commented Jul 25, 2023

Add strict mode to fail the Gradle task in case of parsing errors #67

Add strict mode to fail the Gradle task in case of parsing errors #67

Conversation

SimonMarquis commented Jul 22, 2023

jamesonwilliams left a comment

Choose a reason for hiding this comment

autonomousapps left a comment

Choose a reason for hiding this comment

autonomousapps left a comment

Choose a reason for hiding this comment

autonomousapps Jul 24, 2023

Choose a reason for hiding this comment

SimonMarquis Jul 24, 2023

Choose a reason for hiding this comment

SimonMarquis Jul 24, 2023 • edited

Choose a reason for hiding this comment

autonomousapps Jul 24, 2023

Choose a reason for hiding this comment

SimonMarquis Jul 24, 2023

Choose a reason for hiding this comment

SimonMarquis Jul 24, 2023

Choose a reason for hiding this comment

autonomousapps Jul 25, 2023

Choose a reason for hiding this comment

SimonMarquis commented Jul 25, 2023

SimonMarquis Jul 24, 2023 •

edited