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

Refactor fileeditor-extension for modularization #6904

Merged
merged 15 commits into from Sep 12, 2019

Conversation

@ajbozarth
Copy link
Contributor

@ajbozarth ajbozarth commented Jul 25, 2019

References

Part of the ongoing Issue #6901

Code changes

An initial POC for modularizing the features added in the activate function to allow other extensions to call them. This initial PR sets the ground work and includes the modularization of the menu.addItems code block.

User-facing changes

N/A

Backwards-incompatible changes

N/A

An initial POC for modularizing the features added in the activate function
to allow other extensions to call them. This inital PR sets the ground work
and includes the modularization of the `menu.addItems` code block.

Part of the ongoing Issue jupyterlab#6901
@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

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Jul 26, 2019

@jasongrout how does this look (based on our discussion in the issue). I have a set of progressive branches based on this in my fork to pull out each section bit by bit

@jasongrout jasongrout self-requested a review Jul 29, 2019
@jasongrout jasongrout self-assigned this Jul 31, 2019
@@ -64,7 +60,7 @@ const FACTORY = 'Editor';
/**
* The command IDs used by the fileeditor plugin.
*/
namespace CommandIDs {
export namespace CommandIDs {
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.

Let's put these in the commands file or in a separate file so we don't have circular imports (commands imports index, index imports commands)

Copy link
Contributor Author

@ajbozarth ajbozarth Aug 5, 2019

Choose a reason for hiding this comment

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

I can move these in this PR, I had made that change in my one of follow up branch anyway. (I have a set of branches ready with the follow up steps after this PR)

import { CommandIDs } from './index';

export default class Commands {
static addMenuItems(
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.

I would probably take these out of the class and make each menu addition a separate function, then perhaps one overall function that calls each smaller function. That way another extension could simply import and add whichever parts they wanted, or they could add everything.

Copy link
Contributor Author

@ajbozarth ajbozarth Aug 5, 2019

Choose a reason for hiding this comment

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

I can make thing even more modularized, no problem for me. I'll try to make those changes and push them either Monday or Tuesday

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 2, 2019

Thanks, this looks like a great start! I've added a few comments inline.

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Aug 6, 2019

@jasongrout I tried pinging you on bitter for a faster response, but I'll just post my question here instead since you probably missed my ping.

I have a couple of branches in my fork with iterative commits moving the remaining code from index.ts into commands.ts that I made with the intention of creating followup PRs. But I was wondering if you think I should just push those commits to this PR instead. I would still need to do further work to modulize more like you recommend, but wanted to get your opinion prior to my work to prevent merging issues, you can see my branch containing all my iterative commits at https://github.com/ajbozarth/jupyterlab/tree/editorcommands3 (if you think it's a good idea I would just merge those commits into this branch and address comments from there)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 7, 2019

I'm happy to review all the work here in one place. The work is all one conceptual refactor, so probably makes sense to review as a whole.

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Aug 7, 2019

I'll push the commits to this branch then, each commit moves a different chunk of code into commands.ts. Once I push those I'll start work on further modularizing each function into smaller functions as suggested

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 8, 2019

I'll push the commits to this branch then, each commit moves a different chunk of code into commands.ts. Once I push those I'll start work on further modularizing each function into smaller functions as suggested

Sounds good.

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Aug 19, 2019

Merged with latest master and pushed some of my ongoing work to further modularize, feel free to review my commits as I go. This PR will be my focus the next couple days.

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Aug 20, 2019

status update: I've modularized all my code I'd written so far but the addCommands function (which I am starting on). I also added documentation comments for all the functions and vars in commands.ts which should be reviewed and critiqued. Once I modularize addCommands I will continue to pull out and modularize the remaining sections in index.ts of which there are only a few remaining.

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Aug 21, 2019

@jasongrout I've finished extracting and modularizing the extension functionality as you suggested. If you have time to take a look, I'd suggest reviewing commit by commit since the change is overall large. As of now I'm looking at activate() and seeing if there's any further code I can pull into commands.ts

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Aug 21, 2019

I've spent further time in the index.ts and believe the remaining code there should stay there and not be moved to commands.ts if in your review you would like to double check that assessment. As such my work on this PR is finished pending addressing future review comments.

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Sep 3, 2019

@jasongrout this has been quiet for a couple weeks and was wondering if you would have a chance to review it soon? I'll also ping the gitter for some feedback as well, thanks

@jasongrout jasongrout added this to the 1.2 milestone Sep 9, 2019
Copy link
Contributor

@jasongrout jasongrout left a comment

This is looking great, thanks! I have two suggestions in review to simplify the code and expose it to others.


export const markdownPreview = 'fileeditor:markdown-preview';
}
import Commands, { EDITOR_ICON_CLASS, FACTORY } from './commands';
Copy link
Contributor

@jasongrout jasongrout Sep 10, 2019

Choose a reason for hiding this comment

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

I thought the purpose here was that other modules could use Commands. Can you also re-export Commands so that other modules can use it? Something like

Suggested change
import Commands, { EDITOR_ICON_CLASS, FACTORY } from './commands';
export Commands from './commands';
import Commands, { EDITOR_ICON_CLASS, FACTORY } from './commands';

Copy link
Contributor Author

@ajbozarth ajbozarth Sep 10, 2019

Choose a reason for hiding this comment

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

Nice catch, I added the export, but it needed brackets since it was changed to a namespace.

* A utility class for adding commands and menu items,
* for use by the File Editor extension or other Editor extensions.
*/
export default class Commands {
Copy link
Contributor

@jasongrout jasongrout Sep 10, 2019

Choose a reason for hiding this comment

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

Can you make this a namespace instead of a class? It seems that since everything is mostly static functions, it makes more sense to just make it a namespace. Then the private things below just don't get exported, and the normal static functions just become exported functions.

Copy link
Contributor Author

@ajbozarth ajbozarth Sep 10, 2019

Choose a reason for hiding this comment

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

I've updated it to a namespace, it required some tinkering with the private content, but it made all the exported content much cleaner (no more this.)

ajbozarth added 2 commits Sep 10, 2019
Also added a missing export for Commands to be used externally
@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Sep 10, 2019

@jasongrout I've addressed your review comments and resolved the conflicts with master. Are there any other issues or is this ready to merge? Also are there any other sections in index.ts you think should be moved to commands.ts? Personally I think nothing else needs to be moved.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 12, 2019

Thanks, this looks great! I'll wait for tests to pass before merging.

Congratulations on your first jlab contribution!

@ajbozarth
Copy link
Contributor Author

@ajbozarth ajbozarth commented Sep 12, 2019

Thank you @jasongrout hopefully there will be more contributions to come

@jasongrout jasongrout merged commit 1f59d9c into jupyterlab:master Sep 12, 2019
9 checks passed
@ajbozarth ajbozarth deleted the editorcommands branch Sep 12, 2019
ajbozarth added a commit to ajbozarth/jupyterlab that referenced this issue Oct 12, 2019
Due to a still unclear issue, when passing settingRegistry to
functions it changes the beahvior of the commands that use it.

This change moves the six commands that interact with settingRegistry
back to index.ts from commands.ts where they were oved in jupyterlab#6904

This fixes jupyterlab#7295
@lock lock bot locked as resolved and limited conversation to collaborators Oct 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants