Skip to content

Fix diagnostic outputs for Scala 2.12.13 #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

aishfenton
Copy link
Contributor

Description

Adds the correct overrides in DepsReporter to keep diagnostic reporting working in Scala 2.12.13 <= x < 2.13.12.

This is due to SemanticdbReporter calling doReport on it's underlying reporter, rather info0.

Motivation

Currently if you have SemanticDB enabled, and are using Scala 2.12.13 <= x < 2.13.12, you get empty diagnostic report protos.

Related to this issue: #1484

}

@Override
public void doReport(Position pos, String msg, Severity severity) {
Copy link
Contributor Author

@aishfenton aishfenton Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Just doReport and info0 implementation is changed from the after_2.13.12 version.

@eed3si9n
Copy link

eed3si9n commented Dec 2, 2023

Is the current test coverage (https://github.com/bazelbuild/rules_scala/tree/master/test/shell has a bunch of shell scripts) missing the test for Scala 2.12.x on this issue?

@aishfenton
Copy link
Contributor Author

@eed3si9n I think it doesn't test the combination of Semanticdb + Diagnostics. I can add something in to test that combo.

Harder to test is the combinations of < 2.12.13 and >= 2.12.13, as the repo's default dep_providers are for 2.12.18.

@aishfenton
Copy link
Contributor Author

And cc @scoquelin, since this is a follow up to #1522

Copy link
Collaborator

@liucijus liucijus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @aishfenton!

@aishfenton
Copy link
Contributor Author

aishfenton commented Dec 4, 2023

@liucijus I'm adding tests for the cross-product of versions having semanticdb + diagnostics enabled. But I'm finding that having semanticdb enabled also changes the lines numbers that are output (I have no idea how... they're still correct just more similar to 2.13's way of doing it ... but not exactly — (╯°□°)╯︵ ┻━┻).

The cross product of (version X semanticdb x diagnosticReports) is getting a bit nuts to test for. I'm tempted to remove the lines numbers from DiagnosticsReporterTest and instead test for severity + error message. I'm thinking that the specific lines number outputs are just too unstable between plugins/scala-versions to test against.

How would you feel about that?

@aishfenton
Copy link
Contributor Author

Added that change in to support cross-testing @liucijus, but let me know if you want to change it.

@aishfenton aishfenton requested a review from liucijus December 6, 2023 02:22
@simuons simuons merged commit eb63ea0 into bazel-contrib:master Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants