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.

Sign up for GitHub

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

Jump to bottom

Rename FooWidget -> Foo #2166

Closed
wants to merge 19 commits into from
Closed

Rename FooWidget -> Foo #2166

wants to merge 19 commits into from

Conversation

ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 9, 2017 •
edited by blink1073

Fixes #2090

We have decided to not use Widget in the names of Widget subclasses anymore.

@ellisonbg ellisonbg requested a review from afshin May 9, 2017 21:00
@ellisonbg ellisonbg requested a review from blink1073 May 9, 2017 21:00
@ellisonbg
Copy link
Contributor Author

@blink1073 should I rename the npm packages with widget in the name to follow this convention?

@blink1073
Copy link
Member

This looks good to me so far. Agreed that the package (and folder) names should be updated.

@blink1073 blink1073 added this to the Beta milestone May 10, 2017
@ellisonbg
Copy link
Contributor Author

@blink1073 this is a beast of a PR now. Trying to finish up so we can merge without conflicts causing pain. Tests pass locally but are failing on Travis and AppVeyor. Any ideas?

@ellisonbg
Copy link
Contributor Author

Ahh, I see the issue. I have renamed some of the npm packages and the new ones are not on npm. How do we resolve that without a release?

@blink1073
Copy link
Member

We don't update jupyterlab/package.template.json until we are about to publish the python package.

@blink1073
Copy link
Member

(which is done automatically in the npm run publish step in the release notes).

blink1073
blink1073 reviewed May 10, 2017
View reviewed changes
jupyterlab/src/main.ts
@@ -25,17 +25,17 @@ import * as completerExtension
import * as consoleExtension
Copy link
Member

Choose a reason for hiding this comment

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

This file isn't used anymore, it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow, we are using console-extension still. Can you clarify?

blink1073
blink1073 reviewed May 10, 2017
View reviewed changes
packages/theme-light/package.json Outdated
@@ -0,0 +1,59 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

not meant for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed...

blink1073
blink1073 reviewed May 10, 2017
View reviewed changes
test/src/csvwidget/widget.spec.ts
@@ -13,11 +13,11 @@ import {

Copy link
Member

Choose a reason for hiding this comment

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

This file needs a rename

Copy link
Member

Choose a reason for hiding this comment

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

(or deletion it looks like).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed...

@blink1073
Copy link
Member

Do you want to change VdomWidget -> VdomRenderer as in this PR as well?

@ellisonbg
Copy link
Contributor Author

This PR is big enough already, let's wait and do the VDom rename in a follow on. Worried about this generating conflicts.

@ellisonbg
Copy link
Contributor Author

@blink1073 should I revert jupyterlab/package.template.json? How should I get the tests working again on Travis?

@blink1073
Copy link
Member

I'll finish up this PR by making one against yours.

@ellisonbg
Copy link
Contributor Author

ellisonbg commented May 10, 2017 via email

@blink1073
Copy link
Member

Required a rebase, continuing in #2177.

@blink1073 blink1073 closed this May 10, 2017
@blink1073 blink1073 mentioned this pull request May 11, 2017
Closed
@ellisonbg ellisonbg deleted the no-widget branch June 14, 2017 04:14
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@blink1073 blink1073 blink1073 left review comments

@afshin afshin Awaiting requested review from afshin

Assignees
No one assigned
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Milestone
0.31.0
Development

Successfully merging this pull request may close these issues.

Don't use Widget in names of classes that subclass Widget
2 participants
@ellisonbg @blink1073