Skip to content

Adding interfaces for datascience work (#1) #2688

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

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Sep 25, 2018

  • Working command with service manager

  • Initial interfaces

  • 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!)
  • Has unit tests & system/integration tests
  • Any new/changed dependencies in package.json are pinned (e.g. "1.2.3", not "^1.2.3" for the specified version)
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)

* working without inversify

* Working command with service manager

* Initial interfaces
@rchiodo
Copy link
Author

rchiodo commented Sep 25, 2018

This is just a skeleton of the work we're starting. I imagine we'll be make a ton more changes like this and then we'll take all of them together into master at a later date.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Nothing jumpus out at me, but Don or anyone else on the team may know better. ;)

@rchiodo rchiodo merged commit cda22d1 into microsoft:datascience Sep 25, 2018

@injectable()
export class DataScience implements IDataScience {
constructor(@inject(ICommandManager) private commandManager: ICommandManager,
Copy link
Member

Choose a reason for hiding this comment

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

Note: I've already changed this in my upcoming checkin to instead just inject the service manager and then get the various services that we need off of that.

@@ -105,6 +107,10 @@ export async function activate(context: ExtensionContext): Promise<IExtensionApi
const lintingEngine = serviceManager.get<ILintingEngine>(ILintingEngine);
lintingEngine.linkJupiterExtension(jupyterExtension).ignoreErrors();

// Activate the data science features
const dataScience = serviceManager.get<IDataScience>(IDataScience);
await dataScience.activate(context);

Choose a reason for hiding this comment

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

Is there a reason why this needs to be awaited.
We need to be careful what happens with the extension activation. At the end of the day, this impacts the time taken for the extension to load, i.e. the time the user has to wait before one can use the extension (intellisense, etc).

Suggestion:

  • In the extension activate method (in extensions.ts file), do not await instead do as follows (i.e. invoke activate but do not wait for it to complete: dataScience.activate(context).ignoreErrors()`
  • If you need you can add your error logging in the DataScience.activte method as follows:
    @log('Starting Data Science')
    public async activate(context: ExtensionContext): Promise<void> {
        this.registerCommands();
    }

This will add the necessary logging (trace and exceptions).
Please update your branch with latest master to get the log decorator.

@DonJayamanne
Copy link

DonJayamanne commented Sep 26, 2018

For some reason, the package-lock.json file has been updated unnecessarily. You might want to ensure you have the latest version of npm installed. (version >= 6)

Try installing using npm ci.

We need to ensure package-lock.json doesn't get updated unnecessarily, as the versions of the packages are locked in there and the versions released with the extension are in the 3rd party licenses file (https://github.com/Microsoft/vscode-python/blob/master/ThirdPartyNotices-Distribution.txt).

Suggestion:

  • Revert the change to the package-lock.json.
  • Will make it easier when merging changes upstream to master branch

@DonJayamanne
Copy link

Apologies for the late review.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants