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

Changed public API for execution to return an object and provide a callback which is called when interpreter setting changes #12597

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

karrtikr
Copy link

@karrtikr karrtikr commented Jun 26, 2020

For #12596

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

* Returns the Python execution command corresponding to the specified resource, taking into account
* An event that is emitted when the interpreter configuration changes.
*/
readonly onDidChangeExecDetails: Event<Uri | undefined>;

Choose a reason for hiding this comment

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

I'd rename ExeDetails to something else.
The comments are related to interpreter configuration, but event has work exec.
E.g. why not onDidChangeInterpreterDetails

Choose a reason for hiding this comment

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

or at a minimum onDidChangeExecutionDetails to be consistent with Execution in other API. Else feels weird to use exec in one place and execution in another (specially for PUblic API).

On a side note, not sold on the word execution is a state of a current process, where as we're dealing with the environment that has been selected (not its state).

Copy link
Author

@karrtikr karrtikr Jun 29, 2020

Choose a reason for hiding this comment

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

E.g. why not onDidChangeInterpreterDetails

Because an API method getExecutionDetails exists, it only makes sense to have a change event API associated with the method, which is why having onDidChangeExecutionDetails makes more sense.

The comments are related to interpreter configuration, but event has work exec.

It currently only deals with sending interpreter configuration change event, but can change along with changes in getExecutionDetails API. I am thinking to have something like,

"An event that is emitted when execution details (for a resource) change. For instance, when interpreter configuration changes."

Copy link
Author

Choose a reason for hiding this comment

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

On a side note, not sold on the word execution is a state of a current process, where as we're dealing with the environment that has been selected (not its state).

The motivation for using "Execution details" here is : An API which gives all the details the consumer needs to execute code within the environment.

I think having a doc comment should make things clearer.

@karrtikr karrtikr requested a review from DonJayamanne June 29, 2020 13:58
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.2% 1.2% Duplication

@karrtikr karrtikr requested review from kimadeline and removed request for ericsnowcurrently June 29, 2020 16:00
@karrtikr karrtikr merged commit 9891723 into microsoft:master Jun 29, 2020
@karrtikr karrtikr deleted the newapi branch June 29, 2020 17:37
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.

4 participants