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

[Dummy] Support to provide advices #1

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fcollonval
Copy link
Collaborator

No description provided.

Should be defined only in one place (preferably in pyproject.toml)
Comment on lines -116 to -125
"jupyter-releaser": {
"hooks": {
"before-build-npm": [
"python -m pip install jupyterlab~=3.1",
"jlpm"
],
"before-build-python": [
"jlpm clean:all"
]
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an old heritage of the project template. It should now be specified in pyproject.toml

Comment on lines -49 to -51
export class GalyleoModel
extends CodeEditor.Model
implements DocumentRegistry.ICodeModel
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As you said you actually don't need a specific model, you are simply applying your viewer on a file with a specific extension. Therefore you don't need a custom model,...

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be wonderful (as we both know, the less code, the better :-)). But without a Model and an associated ModelFactory, I don't know how to associate an open command with a specific editor, since (AFAIK) the DocumentRegistry operates on models, not extensions. But you are perfectly right, there should be no code in the GalyleoModel class, and the only code in the GalyleoModelFactory class should be a createNew method, which just returns a GalyleoModel.
If there is a better way to do this, (I am far from an expert), I would love to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I looked at the code again, and now I see what you mean! Thanks

/**
* An implementation of a model factory for base64 files.
*/
export class GalyleoModelFactory extends TextModelFactory {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

..., you don't need a model factory and...

@@ -332,7 +222,7 @@ function activateGalyleo(
fileTypes: ['Galyleo'],
defaultRendered: ['Galyleo'],
defaultFor: ['Galyleo'],
modelName: 'galyleo',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...and you can simply tell the widget factory it needs to use the default text model factory.

@rickmcgeer
Copy link
Contributor

This is awesome! Every line of code I get to delete is a joy!
Many thanks, @fcollonval

@rickmcgeer
Copy link
Contributor

@fcollonval I've created a branch, LTS-Devel (Long Term Sustainable development) where I'm implementing your changes. I'm currently getting hundreds of tsc errors, which just means I've screwed up a config file. If push comes to shove I'll just delete and redo with cookiecutter.
Thanks again

@rickmcgeer
Copy link
Contributor

@fcollonval I've merged your changes into a branch, and I will create a PR and ask you to review, if that is OK. Before I do:

  1. Are you willing?
  2. Is there anything I can/should do to make the review easy on you?

I can't thank you enough for all the help you've given us, and I hesitate to ask for more, but I am hoping that the community will find our extension useful for things other than Galyleo. We called it the "JupyterLab Universal Extension" because the idea was that all we (or anyone else) would have to do to plug in a new web-based editor was write a little glue code on the editor side (it's about 30 lines of code for Galyleo) and simply provide a settings file on the JupyterLab side.
We aren't there yet, but we are close.

@fcollonval
Copy link
Collaborator Author

1. Are you willing?

Sure I can take a look.

2. Is there anything I can/should do to make the review easy on you?

If you could set up a binder for the PR with an example of what to check, it will be great.

@rickmcgeer
Copy link
Contributor

I have it up and running on our beta server, and so probably the easiest thing for you is for me just to give you an account on that (it's an enhanced JupyterHub). All I need is a Google-recognized email address. Is that OK?

@fcollonval
Copy link
Collaborator Author

All I need is a Google-recognized email address. Is that OK?

I sent you a PM on gitter.

@rickmcgeer rickmcgeer mentioned this pull request Feb 24, 2023
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.

2 participants