Conversation
…eport a column dev/scalastyle emits inline GitHub `::error` annotations for scalastyle violations by matching two sbt output shapes. The sbt-logger shape, `[error] <path>:<line>: <message>`, was matched by a regex requiring whitespace immediately after `:<line>:`, on the assumption that the absence of a `:<col>:` is what distinguishes it from a Scala compiler error of shape `[error] <path>:<line>:<col>: <msg>`. That assumption doesn't hold for every checker: the nonascii checker's sbt-logger output includes a column, e.g. `[error] .../StringExpressionsSuite.scala:1065:17: nonascii.message`. That line matched neither regex, so no annotation was emitted for it -- the violation still failed the build correctly, but a contributor had to download the full job log to find it, the exact problem this script exists to avoid. Make the column optional in both regex branches (the native-console-writer shape already had an optional column that was captured but silently discarded) and thread it through to the new `::error ...,col=<n>` parameter GitHub's annotation command supports when present. A genuine Scala compiler error can't reach this code path in the first place: SparkBuild.scala's `enableScalaStyle`/`cachedScalaStyle` deliberately does not attach style checking to `(Compile / compile)`, precisely so a broken compile elsewhere can't cascade into the style job (and vice versa). Verified with a synthetic regex test covering both formats with and without a column, and end-to-end by temporarily introducing a real non-ASCII violation outside an existing `scalastyle:off` block and confirming `dev/scalastyle` now annotates it with file/line/column instead of dropping it silently (reverted before this commit).
Author
|
cc: @uros-b |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
dev/scalastyleemits inline GitHub::errorannotations for scalastyle violations reported through sbt's logger, matching lines of shape[error] <path>:<line>: <message>. The regex required whitespace immediately after:<line>:, on the assumption that the absence of a:<col>:distinguishes it from a genuine Scala compiler error of shape[error] <path>:<line>:<col>: <msg>. That assumption doesn't hold for every scalastyle checker -- thenonasciichecker's sbt-logger output includes a column, e.g.[error] .../StringExpressionsSuite.scala:1065:17: nonascii.message, which matched neither regex branch and so silently got no annotation.This PR makes the column optional in both regex branches (the native-console-writer branch already captured an optional column but discarded it) and threads it through to GitHub's
::error ...,col=<n>annotation parameter when present.A genuine Scala compiler error can't reach this code path:
project/SparkBuild.scala'senableScalaStyle/cachedScalaStyledeliberately does not attach style checking to(Compile / compile), precisely so a broken compile elsewhere can't cascade into the style job (and vice versa) -- see the comment aboveenableScalaStyle.Why are the changes needed?
Without this, a scalastyle violation from a checker that reports a column (e.g.
nonascii) still correctly fails the build, but gets no inline "Files changed" annotation -- exactly the problem this script's annotation logic exists to prevent (per its own comment: "Without this, a violation cascades into ~7 red CI checks... forcing the user to download a full job log to find the actual violation"). A contributor hitting this checker has to download the full job log to find the violation.Does this PR introduce any user-facing change?
No. This only affects GitHub Actions CI annotations for
dev/scalastyle, a contributor-facing tooling script; it doesn't change Spark's behavior, build output, or the pass/fail result of any check.How was this patch tested?
scalastyle:offblock inStringExpressionsSuite.scala, randev/scalastylewithGITHUB_ACTIONS=true, confirmed the violation is now annotated with file/line/column (::error file=...,line=60,col=3,title=Scalastyle::nonascii.message) instead of being silently dropped, then reverted the temporary change.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code claude-sonnet-5
Created as I saw some lint errors across multiple PRs related to this, my PR that was affected : #58946 , it seems transient. Filing a fix anyways.