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

Bug fixes #418

Merged
merged 10 commits into from
Nov 23, 2018
Merged

Bug fixes #418

merged 10 commits into from
Nov 23, 2018

Conversation

MikhailArkhipov
Copy link

@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)

Copy link
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.


public override string Name => _original == null ? base.Name : this._original.Name;
public override string Name => _original == null ? base.Name : _original.Name;
Copy link
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
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.

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
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
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