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

Running Sidebar Extension API #6895

Merged
merged 18 commits into from Sep 16, 2019
Merged

Running Sidebar Extension API #6895

merged 18 commits into from Sep 16, 2019

Conversation

Queuecumber
Copy link
Contributor

@Queuecumber Queuecumber commented Jul 25, 2019

References

Fixes #6876 and #5728

Takeover of PR #6002

Code changes

This PR adds an API for extensions to add their own sessions into the running sidebar

Note

Please look this over carefully as the merge was rather messy and I am not familiar with all package versions. Additionally I am currently getting hit with bug #4619 so I'm unable to run the tests locally, I am going to try to work around that though

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jul 25, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Jul 25, 2019

Does anyone know why that windows test is failing? It doesn't look like something I changed but I'm not sure how to interpret it

@telamonian
Copy link
Member

@telamonian telamonian commented Jul 25, 2019

Does anyone know why that windows test is failing

There's ongoing issues with the JS CI tests. Don't worry about it. I restarted the failed test

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Jul 25, 2019

Ok, in the future is there a way for me to restart it from here so I don't have to bother you guys?

@telamonian
Copy link
Member

@telamonian telamonian commented Jul 25, 2019

You can close and then immediately reopen the PR. You may also be able to directly restart the test by clicking on Details and then restarting the failed tests.

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Jul 25, 2019

OK thanks it looks like all the checks are passing now

@Queuecumber Queuecumber changed the title [WIP] Running Sidebar Extension API Running Sidebar Extension API Jul 26, 2019
@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Jul 26, 2019

I think this is good to go if we can get a review

@jasongrout jasongrout self-requested a review Jul 29, 2019
@jasongrout jasongrout self-assigned this Jul 31, 2019
Copy link
Contributor

@jasongrout jasongrout left a comment

This looks really nice to me. Thanks @recamshak and @Queuecumber!

I've added a few inline notes for some small changes.

packages/running/src/index.tsx Outdated Show resolved Hide resolved
packages/running/src/index.tsx Outdated Show resolved Hide resolved
packages/running/src/index.tsx Outdated Show resolved Hide resolved
packages/running/src/index.tsx Outdated Show resolved Hide resolved
packages/running/src/index.tsx Outdated Show resolved Hide resolved
packages/running/src/index.tsx Outdated Show resolved Hide resolved
*/
name: string;

add(manager: IRunningSessions.IManager): void;
Copy link
Contributor

@jasongrout jasongrout Aug 2, 2019

Choose a reason for hiding this comment

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

How do we remove things we've added? One common pattern we have in the code is to return a disposable, and when you dispose it, it removes the item from the registry. A simpler way might be to just have a remove method.

Copy link
Contributor Author

@Queuecumber Queuecumber Aug 6, 2019

Choose a reason for hiding this comment

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

Disposable sounds like the right way, if not the easy way, can you point me at an example?

Copy link
Contributor Author

@Queuecumber Queuecumber Aug 6, 2019

Choose a reason for hiding this comment

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

Although its possible that the intention was that this would be add-only

Copy link
Contributor

@jasongrout jasongrout Sep 12, 2019

Choose a reason for hiding this comment

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

Sorry for the delayed reply. A good example of this pattern is

addItem(options: IPaletteItem): IDisposable {
let item = this._palette.addItem(options as CommandPalette.IItemOptions);
return new DisposableDelegate(() => {
this._palette.removeItem(item);
});
}

packages/running-extension/src/index.ts Outdated Show resolved Hide resolved
packages/running-extension/src/index.ts Outdated Show resolved Hide resolved
@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Aug 6, 2019

Not sure what just happened to the build but I'm unable to reproduce the error locally

@Queuecumber Queuecumber reopened this Aug 12, 2019
@vidartf
Copy link
Member

@vidartf vidartf commented Aug 12, 2019

This failure seems relevant:

./terminal-extension/src/index.ts(26,10): error TS2305: Module '"../node_modules/@jupyterlab/running/lib"' has no exported member 'IRunningSessionManagers'.
../terminal-extension/src/index.ts(26,35): error TS2724: Module '"../node_modules/@jupyterlab/running/lib"' has no exported member 'IRunningSessions'. Did you mean 'RunningSessions'?

You need to update the version dependencies in the package.json files. Try running

jlpm run integrity

in the repo folder.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 12, 2019

Sorry, that was confusing. I would say make whatever changes are needed to bring it in line with what is in master now, which I think would be accepting the change.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 12, 2019

Oh, never mind. I didn't catch it was the linter adding an extra line. I would accept the change from the linter.

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Aug 12, 2019

OK I pushed the lint changes

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Aug 19, 2019

@jasongrout How are we doing with this?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 19, 2019

Thanks for the ping. I've added it back to my review queue.

Dialog.warnButton({ label: 'Shut Down All' })
]
title: `Shut Down All ${props.manager.name} Sessions?`,
buttons: [Dialog.cancelButton(), Dialog.warnButton({ label: 'SHUTDOWN' })]
Copy link
Contributor

@jasongrout jasongrout Sep 12, 2019

Choose a reason for hiding this comment

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

Sorry, I meant to suggest adding a space: "SHUT DOWN"

Suggested change
buttons: [Dialog.cancelButton(), Dialog.warnButton({ label: 'SHUTDOWN' })]
buttons: [Dialog.cancelButton(), Dialog.warnButton({ label: 'SHUT DOWN' })]

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 12, 2019

Sorry for the late reply. The only two outstanding things for me are #6895 (comment) and #6895 (review)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 12, 2019

FYI, I merged master in and resolved the conflicts.

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Sep 15, 2019

@jasongrout thanks for the help there, I've actually never had that happen before, how do I incorporate your merge into my repository to continue development?

@Queuecumber
Copy link
Contributor Author

@Queuecumber Queuecumber commented Sep 15, 2019

OK this should take care of the outstanding requests. I strongly suggest taking a close look at the packaging and things because I've never worked with yarn or typescript before. And sorry for the delay we have a new baby in the house so spare time is very limited right now

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

And sorry for the delay we have a new baby in the house so spare time is very limited right now

I understand! Congratulations!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

I noticed that the staging directory had some changes since the last published version, so I reverted it. It should only be changed at release time. This isn't anything you did, but we might as well take care of it here.

@jasongrout jasongrout merged commit 37984a7 into jupyterlab:master Sep 16, 2019
9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

Thank you again @recamshak and @Queuecumber for contributing!

@@ -323,56 +228,54 @@ export class RunningSessions extends ReactWidget {
/**
* Construct a new running widget.
*/
constructor(options: RunningSessions.IOptions) {
constructor(managers: IRunningSessionManagers) {
Copy link
Member

@vidartf vidartf Sep 17, 2019

Choose a reason for hiding this comment

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

Note: This change seems to require a major version bump of the owning package.

Copy link
Contributor

@jasongrout jasongrout Sep 17, 2019

Choose a reason for hiding this comment

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

I think you're right. I've opened #7219 to track this.

@jasongrout jasongrout removed this from the 1.2 milestone Sep 19, 2019
@jasongrout jasongrout added this to the 2.0 milestone Sep 19, 2019
@lock lock bot added the status:resolved-locked label Oct 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants