Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Bug fixes#418

Merged
MikhailArkhipov merged 10 commits intomicrosoft:masterfrom
MikhailArkhipov:master
Nov 23, 2018
Merged

Bug fixes#418
MikhailArkhipov merged 10 commits intomicrosoft:masterfrom
MikhailArkhipov:master

Conversation

@MikhailArkhipov
Copy link
Copy Markdown

@MikhailArkhipov MikhailArkhipov commented Nov 19, 2018

Fixes #397 (remove name protocol from tuple)
Fixes #383 (do not return nulls, return empty sets)
Fixes #378 (null check)
Fixes #379 (explicit protocol comparison)

Comment thread src/LanguageServer/Impl/Implementation/Server.WorkspaceSymbols.cs Outdated
Comment thread src/Analysis/Engine/Impl/Values/ProtocolInfo.cs Outdated
Comment thread src/LanguageServer/Impl/Implementation/Server.WorkspaceSymbols.cs Outdated
Comment thread src/Analysis/Engine/Impl/ModuleTable.cs Outdated
Copy link
Copy Markdown
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Unless there's something else you want to address, LGTM.

Comment thread src/LanguageServer/Impl/Implementation/Server.WorkspaceSymbols.cs Outdated
Comment thread src/LanguageServer/Impl/Implementation/Server.WorkspaceSymbols.cs Outdated
Comment thread src/Analysis/Engine/Impl/Values/SpecializedNamespace.cs Outdated
public override IEnumerable<ILocationInfo> Locations => _original.Locations?.MaybeEnumerate();

public override string Name => _original == null ? base.Name : this._original.Name;
public override string Name => _original == null ? base.Name : _original.Name;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why in this case we call base.Name, and in previous explicitly return empty enumerable instead of calling base.Locations, which also returns empty enumerable?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good question. No idea.

Comment thread src/Analysis/Engine/Impl/Values/ProtocolInfo.cs
return Enumerable.Empty<ILocationInfo>();
}
return defns.Definitions.Select(l => l.GetLocationInfo()).Where(l => l != null);
return defns.Definitions.Select(l => l.GetLocationInfo()).ExcludeDefault();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like we have two almost identical methods: ExcludeDefaults and WhereNotNull.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes ExcludeDefault came from RTVS extensions, looks like WhereNotNull is local thing

@MikhailArkhipov MikhailArkhipov merged commit 4a00ee8 into microsoft:master Nov 23, 2018
jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants