Skip to content
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

Included diagnostics in scip snapshot output #220

Conversation

matthewnitschke-wk
Copy link
Contributor

This concept originally came out of this issue, which was individually reported in sourcegraph/scip within this issue

Adds the option to include indexed diagnostics to the results of scip snapshot

This functionality is explicitly enabled via the --include-diagnostics flag, to support backwards compatibility with current snapshot tests (eg, the inclusion of diagnostics in snapshot files is opt-in)

Example format is as follows:

  • Given this example dart file
    void _unusedElement() {}
  • Running scip snapshot --include-diagnostics will result in
       void _unusedElement() {}
    //      ^^^^^^^^^^^^^^ definition scip-dart pub test_package lib/main.dart _unusedElement().
    //      diagnostic UNUSED_ELEMENT

Test plan

  • I've verified the functionality myself on various indexed scip files, but when attempting to write some unit tests around the new functionality, I was a bit confused on the current test setup, specifically around repolang and how to add diagnostic concepts to it. I'm open to suggestions / contributions to the pr addressing this missing test coverage

@jtibshirani
Copy link
Member

@matthewnitschke-wk thank you for your contribution! I'm sorry we never responded to this, this PR fell through the cracks.

I wasn't able to push to your branch, so instead I opened #226. When it's merged, you'll be listed as a coauthor on the commit.

@matthewnitschke-wk
Copy link
Contributor Author

@jtibshirani Awesome! Thanks for making those changes and polishing it a bit

I'll go ahead and close this pr then

@varungandhi-src
Copy link
Contributor

Hey @matthewnitschke-wk apologies for dropping the ball here. If you have a PR that isn't reviewed within a few days, feel free to @ me.

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.

3 participants