Fail Spotless formatting check before tests execute #487
Conversation
By default, the spotless Maven plugin binds its check goal to the verify phase (late in the lifecycle, after integration tests). Because we currently only run `mvn test` for CI, it doesn't proceed as far as verify so missed formatting is not caught by CI. This binds the check to an earlier phase, in between test-compile and test, so that it will fail before `mvn test` but not disrupt your dev workflow of compiling main and test sources as you work. This strikes a good compromise on failing fast for code standards without being _too_ nagging. For the complete lifecycle reference, see: https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html
ches
requested review from
davidheryanto,
khorshuheng,
pradithya,
woop and
zhilingc
as code owners
|
/lgtm |
|
Not sure what assigning PRs means for the workflow with the bot. @davidheryanto are we waiting for an |
|
I don't think we have any special use of the |
I guess
but apparently giving a Anyway, I'm just wanting people to understand what the next step is on PRs, what they're waiting on—myself included 😅 This PR is small and a quality-of-life thing for all, I hope—how do we land it? (I'm trying to digest #466 because we had to make a similar hack for Spark, and the changeset is a lot larger than the necessary scope because it caught up old missed formatting. So I scratched an itch I've had since December 12th, apparently 😄). |
@ches, you can have a look at the "descriptions" for these commands over here: http://prow.feast.ai/command-help From my understanding, the |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
That makes sense, thanks. The command descriptions didn't help me much, at least for |
|
Ah, the other doc that the bot links to covers the distinction/intent for review process better. Read that some time ago, but I guess it's sinking in now… |
* Fail formatting check before tests execute By default, the spotless Maven plugin binds its check goal to the verify phase (late in the lifecycle, after integration tests). Because we currently only run `mvn test` for CI, it doesn't proceed as far as verify so missed formatting is not caught by CI. This binds the check to an earlier phase, in between test-compile and test, so that it will fail before `mvn test` but not disrupt your dev workflow of compiling main and test sources as you work. This strikes a good compromise on failing fast for code standards without being _too_ nagging. For the complete lifecycle reference, see: https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html * Apply spotless formatting (cherry picked from commit 6363540)
* Fail formatting check before tests execute By default, the spotless Maven plugin binds its check goal to the verify phase (late in the lifecycle, after integration tests). Because we currently only run `mvn test` for CI, it doesn't proceed as far as verify so missed formatting is not caught by CI. This binds the check to an earlier phase, in between test-compile and test, so that it will fail before `mvn test` but not disrupt your dev workflow of compiling main and test sources as you work. This strikes a good compromise on failing fast for code standards without being _too_ nagging. For the complete lifecycle reference, see: https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html * Apply spotless formatting (cherry picked from commit 6363540)
What this PR does / why we need it:
Moves the
spotless:checkMaven goal to execute after tests compile, but beforetestexecutes the tests. This way the check will be run on CI, which runsmvn test, but it doesn't disrupt development flow by failing before compilation (like thevalidatephase would). This strikes a good compromise on failing fast for code standards without being too nagging.By default, the spotless plugin binds its check goal to the
verifyphase (late in the lifecycle, after integration tests). Because we currently only runmvn testfor CI (andpackagewith-DskipTestsfor E2E), it doesn't proceed as far asverifyso changes that don't comply get merged. Then, when someone cleans up at a later date by runningspotless:applyon their branch, a lot of noisy, unrelated changes come into review.Fortunately a recent PR did run
spotless:apply, when I run it on this branch the changes are few so it won't create a lot of conflicts for others' outstanding work.Does this PR introduce a user-facing change?: