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

Update the api.ts for the getActiveFolderPath method. #3528

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

gcampbell-msft
Copy link
Collaborator

@gcampbell-msft gcampbell-msft commented Jan 9, 2024

This will need to also update and pull down the 1.1 version of the API, so this is pending on a new version being published of https://www.npmjs.com/package/vscode-cmake-tools?activeTab=readme.

These changes are thanks to @Kemaweyan, who started this idea here: #3520. This makes the changes in our formal API.

We add the API method implementation and we update the dependency on the public API interface.

See the API PR here: microsoft/vscode-cmake-tools-api#3

This will need to also update and pull down the 1.1 version of the API.
Changes coming.
@gcampbell-msft
Copy link
Collaborator Author

Make sure to update the api version in the class.

@gcampbell-msft
Copy link
Collaborator Author

Npm package was updated for the API, as well as the dependency here, so this is ready to review. I confirmed that the API works by using the VS Code API Explorer (@benmcmorran)

@gcampbell-msft
Copy link
Collaborator Author

@benmcmorran @snehara99 ping. Thanks!

benmcmorran
benmcmorran previously approved these changes Jan 16, 2024
@@ -12,7 +12,7 @@ import { assertNever } from '@cmt/util';
export class CMakeToolsApiImpl implements api.CMakeToolsApi {
constructor(private readonly manager: ExtensionManager) {}

version: api.Version = api.Version.v1;
version: api.Version = api.Version.v2;
Copy link
Member

Choose a reason for hiding this comment

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

It's not a huge deal given what I expect is pretty low usage of the API, but we can do better by dynamically selecting which version of the API this claims to be based on the version that was requested from getApi() call.

As is, the extension will always return a v2 API object, even if the caller requested v1. Because v2 is backwards-compatible with v1, there should be no harm in simply overriding the API version in the returned object to be v1 when v1 is requested. That will allow any existing exact match queries for the v1 version of the API to continue to succeed even after the 1.17 release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this, good idea, I'll modify.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made the changes, let me know if you think I should do it in a different way @benmcmorran . Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@benmcmorran Ping.

@gcampbell-msft gcampbell-msft enabled auto-merge (squash) January 18, 2024 19:09
@gcampbell-msft gcampbell-msft merged commit 0c3de56 into main Jan 19, 2024
4 checks passed
@gcampbell-msft gcampbell-msft deleted the dev/gcampbell/UpdateApi branch January 19, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants