Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove OptionSet and Workspace from TypeScript and F# Inline Rename interfaces #58381

Merged
merged 7 commits into from
Jan 12, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented Dec 16, 2021

Replaces TS and F# external interfaces with equivalent abstract classes (with updated signatures) to allow easier modifications in future.
Minor fix in OmniSharpRenamer

F#:
dotnet/fsharp#12527

TypeScript:
https://devdiv.visualstudio.com/DevDiv/_git/TypeScript-VS/pullrequest/370211

@tmat tmat requested a review from a team as a code owner December 16, 2021 23:36
@jasonmalinowski jasonmalinowski self-requested a review December 18, 2021 02:35
@tmat tmat force-pushed the TSInlineRenameInfo branch from 382211b to 8bd26b5 Compare December 19, 2021 21:29
@tmat tmat requested a review from a team as a code owner December 19, 2021 21:54
@tmat tmat changed the title Remove OptionSet and Workspace from IVSTypeScriptInlineRenameInfo Remove OptionSet and Workspace from TypeScript and F# Inline Rename interfaces Dec 19, 2021
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I get what this PR is doing, but I do wish the Obsolete messages generally pointed to their replacements, or at least to a tracking bug or something that when closed means all of that stuff could be deleted. Otherwise I imagine us leaving a bunch of this around or later being confused what the old stuff is for.

@tmat
Copy link
Member Author

tmat commented Dec 21, 2021

I get what this PR is doing, but I do wish the Obsolete messages generally pointed to their replacements, or at least to a tracking bug or something that when closed means all of that stuff could be deleted. Otherwise I imagine us leaving a bunch of this around or later being confused what the old stuff is for.

I can file a bug: #58442. I don't think adding messages adds any value since I'm already updating TS and F# as well.

@tmat
Copy link
Member Author

tmat commented Jan 5, 2022

@jasonmalinowski Any more feedback?

@tmat tmat enabled auto-merge (squash) January 5, 2022 05:51
@jasonmalinowski
Copy link
Member

I don't think adding messages adds any value since I'm already updating TS and F# as well.

I'd disagree, simply because we've had a bunch of times where we create a replacement, the consuming side updates, and we then forget to actually do the follow-up. I appreciate the tracking bug but unless it's listing what is actually obsolete it's a huge mess to sort out later if you're either not the original author, or it's been long enough you have to page it back into memory.

@tmat
Copy link
Member Author

tmat commented Jan 8, 2022

. I appreciate the tracking bug but unless it's listing what is actually obsolete it's a huge mess to sort out later if you're either not the original author, or it's been long enough you have to page it back into memory.

The issue links to this PR. It's easy to look at the diff and remove the types that are marked Obsolete here.

@tmat tmat merged commit d34406e into dotnet:main Jan 12, 2022
@ghost ghost added this to the Next milestone Jan 12, 2022
@tmat tmat deleted the TSInlineRenameInfo branch January 12, 2022 01:23
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants