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

Extension reloaded #16

Merged
merged 69 commits into from
Mar 8, 2021
Merged

Extension reloaded #16

merged 69 commits into from
Mar 8, 2021

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Feb 26, 2021

I'm working on rebooting this extension. Should this extension keep living in the jupyterlab organization? Could I be granted right to publish it on pypi (cc @blink1073 @jaipreet-s)?

TODO

  • Add support for GitLab
    • Update GitHub for new discussion data structure
  • More server configuration (base url, token, third service)
  • Switch to codemirror for plain text (for Jupyter ecosystem coherence)
  • Refresh the code
  • Extend testing
  • Switch to GitHub workflow
  • UI improvements
    • Use virtual tree view for PR/file list
    • Better file path display
    • Support markdown rendering
    • Support discussion at PR level

@blink1073
Copy link
Member

Should this extension keep living in the jupyterlab organization?

We don't have a process to remove a repo, worth bringing up at the JupyterLab developer's meeting

Could I be granted right to publish it on pypi

Done!

@bollwyvl
Copy link
Contributor

Thanks for sharing! What's totally 🤯 is i had independently started punching on this yesterday (branch, self-pr, binder, but...). As this was looking so dead, I didn't much consider keeping it upstream-compatible, but if other folks want to generally work together to warm it up, I'm game.

Screenshot from 2021-02-26 07-53-52

field report:

  • lab3, all the way, but...
    • situation still not great with nbdime/jupyterlab-git
  • I did get the monaco stuff back up and running with all the fixin's in lab3/wp5, and that is a really nice diff UI. But I'll agree: if CodeMirror works, we should use it, as what I had to do was not worth the complexity.
  • Wiring up the markdown rendering is indeed clutch.

This work was a prelude to the near-term thing for me is also multiple backends.

Thinking just about quick wins for GitHub for a sec, though:

  • for binder/demo purposes, having a read-only pr view that didn't require auth would be clutch.
    • I don't know how feasible it is to scrape the HTML
    • looks like there isn't an RSS feed of pull requests
    • there are external proxy services, but that's a much more unacceptable single-point-of-failure
  • other githubs e.g. a github enterprise (seems like not a huge lift, set an env_var, and off you go)
  • otherwise smooth the auth experience:
    • this doesn't actually seem like a great use case for user settings
      • it's too easy to have a password end up someplace you don't want it

But more generally:

  • the data contract between the frontend and backend is not very strict, and leads to lots of ambiguity/foo?: any
    • store the canonical typings in a JSON schema, and have the front- and back-end code-gen'd or just use jsonschema/ajv
      • i prefer this to having either the front or back end be the source of truth (e.g. pydantic or ts-json-schema-generator), and using the schema to test both
    • anyhow, if writing schema
      • documenting what's there, and works, is valuable,
      • but there has to be something already out there
  • handle multiple providers, even of the same provider type, with different creds, in the same lab session
    • keep /pullrequests/gh/ enabled by default (but better still, one that isn't broken-by-default)
    • make the python-side Manager extensible, either with traitlets or setuptools to enable multiple configurations
PullRequests:
  providers:
    gh:
      module: jupyterlab_pullrequests.providers.GitHub
      conf:
        github_api_url: https://api.github.com
    gl:
      module: jupyterlab_pullrequests.providers.GitLab
      conf:
        gitlab_api_url: https://api.gitlab.com

@fcollonval
Copy link
Member Author

Thanks for the comment and feedback @bollwyvl

I should have a first satisfactory version by the end of the week. Here is the current status:
image


Here are some comments on points you raised earlier.

other githubs e.g. a github enterprise (seems like not a huge lift, set an env_var, and off you go)

For now I put the base API url as configurable. But I'm wondering if we should not change completely the logic and show the PRs associated with the Git remote of the current active folder in the browser - and then allow the user to set the URL if the extension can't figure out what it is from the remote URL.

otherwise smooth the auth experience:
this doesn't actually seem like a great use case for user settings
it's too easy to have a password end up someplace you don't want it

I definitely agree on this one. My dream would be to get a generic credentials provider (like they did in VS Code) that extensions can use to request/store credentials. That could be built on top of keyring to support OS storage as well as other backends through configuration.

Extension such as @jupyterlab/git or @jupyterlab/github would benefit of such credentials support too ... the future RTC feature may be need it too.

the data contract between the frontend and backend is not very strict, and leads to lots of ambiguity

Agree this would make life easier - but I'm still looking for the best tool for that.

handle multiple providers, even of the same provider type, with different creds, in the same lab session

👍

CodeMirror as comment editor

Done thanks for the suggestion

markdown renderer

As the JupyterLab renderer is used, from my understanding jupyterlab-markup will be used as soon it is installed. So it may be the path forward for user in need for more advanced markdown. But as you said, needs ill be backend provider dependent.

General look and feel

For now, I focus on being homogeneous with the git extension. I must admit that I'm biased toward VS Code design. So having the files in the tree view looks natural...

Regarding the suggestion of a scrollable view with all files, I'm fearing performance issues when dealing with large files.

@bollwyvl
Copy link
Contributor

bollwyvl commented Mar 5, 2021

Agree this would make life easier - but I'm still looking for the best tool for that.

Some I ship with are:

However, json-schema-to-ts's FromSchema looks very interesting, and would remove some entropy coupling. We also have some @deathbeds stuff cooking, but more to be done there...

For this kind of thing, I've actually considered moving the whole js shooting match inside a python package, so that the schema-at-rest doesn't have to get moved around, can never be out-of-sync, and changes are picked up by normal watcher tools.

@bollwyvl
Copy link
Contributor

bollwyvl commented Mar 5, 2021

generic credentials provider ...

Yeah, well... it's pretty darned hard with a low trust deployment target. I really would love if some of these had fallback methods that allowed you to use standards, but pull requests definitely don't have that. But yeah, we shouldn't all be building our own identity manager

keyring

It's important this extension works locally, on the desktop, but it's really important it works in Hubs, docker containers, etc... so keyring kinda falls down. If anything, doing it seriously and properly might require a browser extension (e.g. smart cards used in hospitals, with expensive microscopes, etc). Trying to get creds into vscode things has been... not one of my favorite experiences.

@bollwyvl
Copy link
Contributor

bollwyvl commented Mar 5, 2021

CodeMirror as comment editor

Done thanks for the suggestion

Looking super rad 🚀 . A few things (you just tell me when to stop! 😊):

  • the file diff

    • the big popout boxes look good in the file view
    • i feel like the extra-thick blue lines coming out of the plus could be just a single pixel thick, and probably not jp-brand-color0 all the time
  • in the PR summary,

    • the chat messages could be a lot more condensed and probably don't need any border at all, nor are extra layout colors needed...
    • the PR source/target repo and key (this could be the link instead of the View Details button,
      • e.g. [octocat]jupyterlab/pull-requests#16)
        • the provider could hoist this SVG instead of cooking it into the typescript build
    • the proposer (also with a link to their page)
    • a tall tree or wide miller columns of the files that are actually in that pr that open the tear-aways, with better metadata than +/-
      • the file browser is just going to need a lot of work, and really needs first-class, first-party support, potentially via that datagrid merged cell PR on lumino (think i mentioned above)
    • the "start a new discussion" box could always be visible, and stuck to the bottom of the tab
      • making it automatically scale to what you're writing would be fly, but a SplitPanel would be fine, too
    • i don't know if the "reply" on a past message makes much sense, unless the provider actually supports them (GH sure doesn't, buh)
    • i'd expect too see the why changing metadata in the summary, with a link to open that diff you are seeing

@fcollonval
Copy link
Member Author

Thanks for the feedback @bollwyvl

As I get pressure to deploy it and that the current state is usable. I propose to merge and release for JLab2. Then I'll update to JLab3 and we can start splitting enhancements in multiple PR.

What do you think?

@bollwyvl
Copy link
Contributor

bollwyvl commented Mar 6, 2021 via email

Set remaining points as issues
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.

None yet

3 participants