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

Finalizing design of proposed API for python environments #19841

Merged
merged 60 commits into from
Sep 26, 2022
Merged

Conversation

karrtikr
Copy link

@karrtikr karrtikr commented Sep 16, 2022

Closes #19101 closes #18973

@karrtikr karrtikr added the feature-request Request for new features or functionality label Sep 16, 2022
@karrtikr karrtikr force-pushed the proposedenvapis branch 4 times, most recently from dcffa32 to b29e964 Compare September 19, 2022 23:22
* Carries environments found by the extension at the time of fetching the property. Note a refresh might be
* going on so this may not be the complete list.
*/
readonly environments: Environment[];
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called all? E.g today the fully qualified name is pythonExtApi.environment.environments which is a bit funny. IMO environment.all or something similar reads more fluent

Copy link
Member

Choose a reason for hiding this comment

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

also strongly suggesting to mark this a readonly-array via readonly Environment[] so that extensions don't start to the remove/add elements. Guard the implementation via Object.freeze

Copy link
Author

Choose a reason for hiding this comment

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

Yep, agreed.

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

Great progress. Thanks! Left some nit and suggestions. A broader observation is to mark data that the API gives to consumers as readonly, like readonly-properties and readonly-arrays. That prevents accidential modifications of the API's state

@karrtikr karrtikr closed this Sep 24, 2022
@karrtikr karrtikr reopened this Sep 24, 2022
@karrtikr
Copy link
Author

I'm quite happy with how this has turned out, thanks for all the help! We eventually plan to publish this as a npm types package so it's easier for other extensions to access this.

@karrtikr karrtikr marked this pull request as ready for review September 26, 2022 20:46
@karrtikr karrtikr merged commit 0f04578 into main Sep 26, 2022
@karrtikr karrtikr deleted the proposedenvapis branch September 26, 2022 23:40
eleanorjboyd pushed a commit to eleanorjboyd/vscode-python that referenced this pull request Oct 4, 2022
wesm pushed a commit to posit-dev/positron that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
3 participants