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

Rename 'textSearchProviderNew' #226813

Open
roblourens opened this issue Aug 27, 2024 · 6 comments
Open

Rename 'textSearchProviderNew' #226813

roblourens opened this issue Aug 27, 2024 · 6 comments
Assignees
Labels
polish Cleanup and polish issue search Search widget and operation issues
Milestone

Comments

@roblourens
Copy link
Member

Testing #226669

I recommend removing the New name from types before you start asking extensions to adopt the new API for real, to avoid the pain of handling unnecessary renames

@andreamah
Copy link
Contributor

The problem is that moving the new APIs to the old names is tricky. TextSearchResult and TextSearchContext are now classes instead of interfaces. If people need to change the name for that anyways, I'm not sure if it's worth changing over others and making a half-transition.

@roblourens
Copy link
Member Author

That doesn't seem like a problem, the interface doesn't exist at runtime, you can delete or rename old interfaces and expose the new classes without affecting any existing extensions.

@andreamah
Copy link
Contributor

I think I had difficulty overloading the types when editing https://github.com/microsoft/vscode/blob/main/src/vs/workbench/api/common/extHost.api.impl.ts#L1510.

Is it possible to assign TextSearchMatch to either an interface or class here?

@roblourens
Copy link
Member Author

The interfaces don't show up in extHost.api.impl, plus you can delete the old interfaces.

@andreamah
Copy link
Contributor

The interfaces don't show up in extHost.api.impl, plus you can delete the old interfaces.

But I need to state TextSearchContext in extHost.api.impl, since it's now a class. But then it overloads the type of vscode.TextSearchContext. Would I need to call the old one TextSearchContextOld, then?

@roblourens
Copy link
Member Author

You could delete the old one, since nobody should be writing new code that references it.

@andreamah andreamah added search Search widget and operation issues polish Cleanup and polish issue labels Sep 3, 2024
@andreamah andreamah added this to the September 2024 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
polish Cleanup and polish issue search Search widget and operation issues
Projects
None yet
Development

No branches or pull requests

3 participants
@roblourens @andreamah and others