dotnet / interactive

.NET Interactive combines the power of .NET with many other languages to create notebooks, REPLs, and embedded coding experiences. Share code, explore data, write, and learn across your apps in ways you couldn't before.
MIT License
2.8k stars 374 forks source link

Disallow empty collections in events #3439

Open shyamnamboodiripad opened 5 months ago

shyamnamboodiripad commented 5 months ago

Some events such as DiagnosticsProduced and HoverTextProduced have constructors that throw when the supplied collection of diagnostics / content is empty.

Others such as CompletionsProduced, SignatureHelpProduced and ValueInfosProduced allow this.

Allowing empty collection in events is problematic because it introduces two ways to convey absence of data that consumers have to remember to handle - a) absence of event in the returned set of events for a command and b) presence of event with empty collection. It seems ideal to completely disallow creating events with empty collection so that consumers only need to check whether or not the event was produced (i.e. a)). See this discussion in https://github.com/dotnet/interactive/pull/3426#discussion_r1466781258

This PR fixed the notebook extension to handle absence of event correctly for all language service commands: #3427

It would be great to audit all existing event that include collections to make sure that if empty collections are allowed in some event, there is a legitimate reason why they are allowed.

jonsequitur commented 5 months ago

I don't believe empty collections should be allowed in any of these events. It introduces an extra state that's semantically equivalent (to not publishing the event at all) and pushes cyclomatic complexity onto consumers.