implemented @deprecated diagnostics reporting#430
Conversation
|
@varungandhi-src this PR should be ready for review. Happy to punt on the idea if you'd prefer the scip-typescript indexer not providing this as well |
varungandhi-src
left a comment
varungandhi-src
left a comment
There was a problem hiding this comment.
Overall I think it's fine to add support for features like this; left some inline comments on specifics.
| new scip.scip.Diagnostic({ | ||
| severity: scip.scip.Severity.Information, | ||
| code: 'DEPRECATED', | ||
| message: deprecatedTag.text?.map(part => part.text).join(''), |
There was a problem hiding this comment.
I don't quite understand when we'll have multiple elements here, and why it's justified to join with '' instead of say ' '. Could you explain that? It'd be good to add a code comment for this.
There was a problem hiding this comment.
The result of getJsDocTags() provides a JsDocTagInfo which contains a text field on it correlating to a fully tokenized version of the jsdoc
Because the individual parts of this include spaces, we simply join('') to generate the full tag text
I've added a comment to explain this better (as well as added a test covering this tokenizing better)
| new Foo() | ||
| // ^^^ reference diagnostics 0.0.1 `index.ts`/Foo# | ||
| // diagnostic Information: | ||
| // > This class is deprecated |
There was a problem hiding this comment.
It'd be good to add some test cases for multiline messages with @deprecated to capture whether they work/don't work, and make sure that the final formatting is correct.
| class Foo {} | ||
| // ^^^ definition diagnostics 0.0.1 `index.ts`/Foo# | ||
| // diagnostic Information: | ||
| // > This class is deprecated |
There was a problem hiding this comment.
I don't have a strong feeling on this (we can change it later), but I'm wondering if it makes sense to emit this at the definition site as well. What does the Dart indexer do? I'm thinking of downstream functionality, and it generally doesn't make sense to flag warnings for definitions that are deprecated, but only at usage sites.
There was a problem hiding this comment.
Dart omits the diagnostic on the definition side: https://github.com/Workiva/scip-dart/blob/master/snapshots/output/diagnostics/lib/main.dart#L21
I'll update accordingly
|
|
||
| function main() { | ||
| new Foo() | ||
| bar() |
There was a problem hiding this comment.
Let's add a reference to car() to trigger the logic for multiline diagnostics formatting.
Amp-Thread-ID: https://ampcode.com/threads/T-0319390d-8526-40e6-b85d-aba74c0e2522 Co-authored-by: Amp <amp@ampcode.com>
2 participants
Problem/Solution
Currently the diagnostics field on the reported index file is always empty, regardless on if the typescript indexer detects any diagnostic issues
This information can be especially helpful analysis of a codebase, where questions like "how many deprecations are being used within this package" are asked. This is something that we rely pretty heavily on in scip-dart use cases (which fully indexes all analyzer results)
This PR updates the scip-typescript indexer to start indexing
@deprecatedjsdoc comments, and assigns the correctDeprecateddiagnostic tag. EventuallyDiagnosticTag.Unnecessaryshould be implemented, but this is less beneficial from my point of viewTest plan
A new
diagnosticssnapshot test was added. This include updating the snapshot generation to be in line with the results from the official scip clisnapshotcommand: scip-code/scip#226