rokucommunity / brighterscript

A superset of Roku's BrightScript language
MIT License
160 stars 46 forks source link

Remove `.file` prop from `BsDiagnostic` #1155

Closed TwitchBronBron closed 2 months ago

TwitchBronBron commented 5 months ago

When #1136 lands, we should consider removing file from the BsDiagnostic interface. It doesn't feel right being attached to a diagnostic, and we could easily update DiagnosticsManager to support returning a new object that includes the scope/file information (i.e. the file) for a diagnostic) if we need that information.

This would be a breaking change, so likely should be done as part of v1.

markwpearce commented 3 months ago

Not really sure how this works. How do we associate with a diagnostic with a file? Do we use the Location property on the Diagnostic.relatedInformation?

TwitchBronBron commented 3 months ago

All diagnostics flow through the diagnosticManager now. So file would be a required property when registering a diagnostic. Something like this:

program.diagnostics.register({
    ...DiagnosticMessages.cannotFindName('someVar'),
}, {
    file: theFile,
});

Then if we need to know what file that diagnostic comes from, that would be an attribute of the return value from the diagnostic manager. Perhaps something like this?:

diagnosticManager.getDiagnosticsByFile(); // Map<BscFile, Diagnostic[]>; 
markwpearce commented 2 months ago

We decided that we should replace .file and .range with .location to mirror the changes in the AST Nodes.