jupyterlab / pull-requests

A JupyterLab extension for reviewing GitHub pull requests
BSD 3-Clause "New" or "Revised" License
37 stars 18 forks source link

Extension reloaded #16

Closed fcollonval closed 3 years ago

fcollonval commented 3 years ago

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

blink1073 commented 3 years ago

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 commented 3 years ago

Thanks for sharing! What's totally :exploding_head: 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:

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:

But more generally:

fcollonval commented 3 years ago

Thanks a lot @blink1073; I'll put a reminder to connect at the next meeting.

@bollwyvl thanks for the huge feedback! Before going in more details, here is a screenshot of the current PR status - I broke lots of styles, the input comment on the notebook and POSTing to GitHub. But major things are coming together.

image

fcollonval commented 3 years ago

if other folks want to generally work together to warm it up, I'm game.

Awesome 🚀

lab3, all the way, but... situation still not great with nbdime/jupyterlab-git

I unfortunately needs to publish it for lab2 as well - so I started the reboot on it as updating to lab3 will then be straightforward

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.

Yep this was my feeling to when I tried. So I choose to switch to CodeMirror.

Wiring up the markdown rendering is indeed clutch.

Definitely

For my current reboot, aside of codemirror switch, the big change is to use react only for the sidebar and widget for the tabs as notebook diff, markdown render and co are widget players. This was also the occasion to simplify the notebook diff complexity.

bollwyvl commented 3 years ago

use react only for

buh, yeah, i am not a huge react fan, indeed, but it's better than lumino vdom. It's nice to have for dynamic stuff (as you note) so it may well still be appropriate for the threads as well (which may well change during an interactive session).

however, figuring out the gnarly onComponentDidUpdate integration with the renderer is going to save my life in some places.

the sidebar

the screenshot is too tall
i think the as-is sidebar is super problematic, and much of the interactivity is unintuitive, but these things always are. in mine i went a slightly different way (and don't know why i didn't put in the screenshot) but still feels really information poor.

I am tempted to go even further away from the expanding table, and just make it a list of PRs (with few columns/groups) that launches the main area pr view.

i'd rather see many categories (created, mentioned, filters, orgs, status) than the actual files
Screenshot from 2021-02-26 17-11-50

on the pr view itself: my eye tells me that the narrative of a PR shouldn't be rendered much wider than people usually write it in gh/gl/bb (~80-120 chars), which means there would be enough space for:

So maybe a SplitPanel or something?

As to the viewing of a particular diff: i like the single file tearaway, and it can't really ever be wide enough, ever, for my tastes, if a two up (or three up) view.

of course, the github-style scrollable view with all of the files becomes much more plausible with codemirror vs monaco, so that's probably worth investigating. i probably need to do some whiteboarding!

CodeMirror

are you using it for the thread inputs? would be a shame to textarea when we can codemirror. i didn't get around to it yet. but really, i feel like we're missing a @jupyterlab/threaded-discussion, made of @jupyterlab/cells, but that's a whole other ball of wax.

markdown render

so... i think this is going to be one of those places where the inflexibility of marked really comes into focus. I think we might need to do some thinking about integration with jupyterlab-markup as I could imagine we could put in the right tools to generate site-specific links, e.g. #12 -> /pulls/12, or FOO-123 -> jira/FOO-123. These would necessarily be dependent on the backend provider, and have to be highly configurable, probably as regexen configured in the backend.

I broke lots of styles

not as many as me, i'm sure! i started trying to clean up some of the dom, use more semantic elements, but there's so much crazy stuff going on in there.

publish it for lab2

oh i understand, there are still some lab 1 things i have to support... for the day job. i wish you well, but am super tired of developing/testing lab <3 anything in my free time, much less supporting folk trying to get them to install next to world+dog other extensions (usually plotly) on windows or whatever. so i'll let you do your thing, and keep heckling, and help when it comes time to upgrade :blush:

fcollonval commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

CodeMirror as comment editor

Done thanks for the suggestion

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

fcollonval commented 3 years ago

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 commented 3 years ago

As I get pressure to deploy it and that the current state is usable. I propose to merge and release for JLab2.

Cool go for it!

Then I'll update to JLab3 and we can start splitting enhancements in

multiple PR.

Sure!