Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Move comparer types to public namespace#17437

Merged
weswigham merged 4 commits into
mastermicrosoft/TypeScript:masterfrom
move-comparer-typesmicrosoft/TypeScript:move-comparer-typesCopy head branch name to clipboard
Aug 2, 2017
Merged

Move comparer types to public namespace#17437
weswigham merged 4 commits into
mastermicrosoft/TypeScript:masterfrom
move-comparer-typesmicrosoft/TypeScript:move-comparer-typesCopy head branch name to clipboard

Conversation

@weswigham

Copy link
Copy Markdown
Member

Fixes #16782

TSServer exports some functions which take in comparer functions - functions whose return value is an instance of our Comparison enum. Both Comparer<T> and Comparison were in our internal namespace in core.ts (and thus elided from our .d.ts), but they must be public to create a valid .d.ts for tsserverlibrary, which uses these in its exported signatures.

@ghost

ghost commented Jul 26, 2017

Copy link
Copy Markdown

Should those server utilities be public in the first place?

@weswigham

weswigham commented Jul 27, 2017

Copy link
Copy Markdown
Member Author

@Andy-MS toSortedArray, enumerateInsertsAndDeletes, insertSorted, and removeSorted are public. Given that we require SortedArray<T> and SortedReadonlyArray<T> in a few places on the server, I'd say they're required.

@weswigham

Copy link
Copy Markdown
Member Author

combineProjectOutput should probably also be using Comparer<T>, rather than what it currently is.

@ghost

ghost commented Jul 27, 2017

Copy link
Copy Markdown

I don't think there's a need to allow third-parties to call those, so they could be made internal. @sheetalkamat might be able to confirm.

@weswigham

weswigham commented Aug 1, 2017

Copy link
Copy Markdown
Member Author

@Andy-MS @chuckjaz Added the enumerateInsertsAndDeletes function in #15308, and you factored insertSorted and removeSorted out of inline-in-the-builder into utilities. So what was his and your intent? And combineProjectOutput has been public for a year, so if that were to be moved to use the Comparer<T> type, it would definitely need to be public.

Considering these are helpers for working with SortedArrays, which are on our public interface, I think these can stay public, and the enum and type alias supporting them can be made public, too.

@ghost

ghost commented Aug 1, 2017

Copy link
Copy Markdown

I didn't have any intents with regards to public APIs since I didn't realize I was changing them. It doesn't sound like removeSorted should be a public API since it's just an implementation detail.

@sheetalkamat

Copy link
Copy Markdown
Member

I agree with @Andy-MS as to not have these server utility functions public but to have them as internal instead.

@weswigham weswigham force-pushed the move-comparer-types branch from 890d5a7 to 7dcb5c1 Compare August 1, 2017 23:18
@weswigham

Copy link
Copy Markdown
Member Author

@Andy-MS @sheetalkamat done. I've moved the offending types into an internal half of the namespace.

@sheetalkamat sheetalkamat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave a comment

@weswigham weswigham merged commit 33cc0a1 into master Aug 2, 2017
@ghost ghost deleted the move-comparer-types branch August 2, 2017 13:45
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.