[wip] Add binder #44

bollwyvl · 2021-04-08T16:18:17Z

codecov-io · 2021-04-08T16:20:39Z

Codecov Report

Merging #44 (378432b) into master (909cad7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #44   +/-   ##
=======================================
  Coverage   61.11%   61.11%           
=======================================
  Files          33       33           
  Lines        1638     1638           
  Branches       94       94           
=======================================
  Hits         1001     1001           
  Misses        632      632           
  Partials        5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 909cad7...378432b. Read the comment docs.

bollwyvl · 2021-04-08T16:32:49Z

Does what it says on the tin:

It does pop the nbdime build nag... i'm wondering if we actually need it, or can

disable the nbdime labextension (other stuff seems to initialize)
disable the build message with tornado cruft

bollwyvl · 2021-04-09T15:07:34Z

Took some doing, but things seem to work up to viewing (locally). Will post a Binder screenshot... probably broke a bunch of stuff, and may need to make some separate PRs (that might be good to land before 3.0 final, perhaps).

bollwyvl · 2021-04-09T16:49:45Z

Cool, github-anonymous works all the way up to viewing text diffs, and correctly doesn't try to paginate across all of jupyterlab's pull requests:

I was able to look at about 10 pull requests (and each of their files) before it started dying with 403:

bollwyvl · 2021-04-12T13:59:05Z

@fcollonval Haven't gotten around to warming this up, but the approach has legs, and it's worth checking out the binder.

As this PR has gotten rather large (like i do), I think an effective path forward might be to do some refactoring before trying to land this:

Some of those could go on 3.x...

normalize the python unit test approaches
- some pytest.fixtures would drastically reduce some test boilerplate
use entry_points for the manager providers
- this way, someone could add a your-dvcs-provider-here if they are able to test it better (e.g. bitbucket)
lean more heavily on the PRConfig object rather than fancy names in the manager constructors
introduce some concept of scoped searches
investigate some built-in caching (sadly, there is no tornado-cache a la requests-cache, but let's not go to threadtown unless we have to)
- we could also (optionally) use the tornado curl client, it is generally better, just slightly harder to install on windows

but some of the bigger ones might need to be 4.x...

allow multiple, simultaneous providers

fcollonval · 2021-04-19T06:45:57Z

@fcollonval Haven't gotten around to warming this up, but the approach has legs, and it's worth checking out the binder.

Awesome work @bollwyvl - sorry to not look into this earlier.

As this PR has gotten rather large (like i do), I think an effective path forward might be to do some refactoring before trying to land this

Agreed, I'll help open small PRs.

introduce some concept of scoped searches

vscode GitHub plugin uses the GitHub search API. And the user can customize groups like this:

"githubIssues.queries": [
        {
            "label": "My Issues",
            "query": "default"
        },
        {
            "label": "All Bugs",
            "query": "state:open repo:${owner}/${repository} sort:updated-desc label:type:Bug"
        },
        {
            "label": "All Features",
            "query": "state:open repo:${owner}/${repository} sort:updated-desc label:type:Enhancement"
        }
    ]

We could add a config button in the frontend panel that would open a form allowing the user to customize the search in a similar way. The question being do we want it to be a setting or a state?

investigate some built-in caching (sadly, there is no tornado-cache a la requests-cache, but let's not go to threadtown unless we have to)

What do you think of aiocache?

but some of the bigger ones might need to be 4.x...

allow multiple, simultaneous providers

Yep this definitely needs redesign of the extension code.

fcollonval · 2021-04-19T06:50:33Z

jupyterlab_pullrequests/managers/manager.py

@@ -203,7 +207,7 @@ async def _call_provider(
                            break

                new_ = json.loads(result)
-                if next_url is not None:
+                if has_pagination and next_url is not None:


has_pagination should not be added - but if you have a better API idea it would be great.

As the next_url contains already all required parameters, has_pagination is passed as False when calling the next page (see line 217). But if there are more than 2 pages, requiring has_pagination to be True will block calling the page 3, 4,... .

Unfortunately bringing the pagination in the frontend will be tricky to pull all the discussions.

yeah, unfortuntately i don' think we can just always assume it's okay to pull down 1000 PRs indiscriminantly...

fcollonval · 2021-04-19T06:52:16Z

jupyterlab_pullrequests/managers/gitlab.py

@@ -252,7 +250,7 @@ async def list_prs(self, username: str, pr_filter: str) -> List[Dict[str, str]]:
            )

        # Reset cache
-        self._merge_requests_cache = {}
+        self._merge_requests_cache = None


Why do you reset the cache to None and not an empty dictionary? The idea to set as an empty dictionary is to allow to use the get method line 455.

might be a screwup...

fcollonval · 2021-04-19T06:54:38Z

jupyterlab_pullrequests/managers/github.py

@@ -204,7 +201,7 @@ async def list_prs(self, username: str, pr_filter: str) -> List[Dict[str, str]]:
            )

        # Reset cache
-        self._pull_requests_cache = {}
+        self._pull_requests_cache = None


Why do you reset the cache to None and not an empty dictionary? The idea to set as an empty dictionary is to allow to use the get method line 298.

bollwyvl · 2021-04-19T12:09:04Z

vscode GitHub plugin uses the GitHub search API. And the user can customize groups like this:

anonymous can't use the github search API, so we at least need some approach that is compatible with that, but that's just a subset of the larger problem.

setting or a state?

I'm pretty sure a it should be settings on the frontend, as it would need an "interesting" UI to make it useful, but only the provider knows what the fields are.

Basically, we'll want each search provider to create a JSON schema for "what is a search" that can be mapped to whatever the search provider has, and the provider would have to provide what it knows. So anonymous would only know about:

org (a string)
repo (a string)
state (an enum)

A full github/gitlab/bitbucket/gitea provider knows about the above, and (probably) a ton of other things, plus it could go off and do other queries to provide autocomplete values.

Anyhow, so the client would PUT a body that conforms to that, and then have a search that existed for the duration of the server lifetime. When the server restarts, you'd chunk back into it.

bollwyvl added 2 commits April 8, 2021 12:12

initial binder setup

f836530

badge

378432b

bollwyvl added 2 commits April 9, 2021 11:02

start github_anonymous

45b41a2

update binder with github anonymous, jupyterlab

c888fc0

bollwyvl added 5 commits April 9, 2021 12:10

shotgun more config files

df39800

Update postBuild

b0df9fd

add legacy magic function for notebook

4c702c8

more legacy

b288195

github-anonymous is always anonymous

6a18a7f

bollwyvl added 2 commits April 9, 2021 17:10

rework some test fixtures

f319a19

merge upstream

7dde21a

bollwyvl mentioned this pull request Apr 18, 2021

jupyterlab-pullrequests 3.0.0 conda-forge/jupyterlab-pullrequests-feedstock#6

Merged

5 tasks

fcollonval reviewed Apr 19, 2021

View reviewed changes

This was referenced Apr 19, 2021

Improve config handling #47

Merged

Support legacy notebook launcher #48

Merged

fcollonval added a commit to fcollonval/pull-requests that referenced this pull request Apr 19, 2021

Pull more redesign from jupyterlab#44

67a0657

bollwyvl added a commit to fcollonval/pull-requests that referenced this pull request Apr 20, 2021

revert extra changes from jupyterlab#44

75badfc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Add binder #44

[wip] Add binder #44

bollwyvl commented Apr 8, 2021 •

edited

codecov-io commented Apr 8, 2021

bollwyvl commented Apr 8, 2021

bollwyvl commented Apr 9, 2021

bollwyvl commented Apr 9, 2021

bollwyvl commented Apr 12, 2021

fcollonval commented Apr 19, 2021

fcollonval Apr 19, 2021

bollwyvl Apr 19, 2021

fcollonval Apr 19, 2021

bollwyvl Apr 19, 2021

fcollonval Apr 19, 2021

bollwyvl commented Apr 19, 2021

[wip] Add binder #44

Are you sure you want to change the base?

[wip] Add binder #44

Conversation

bollwyvl commented Apr 8, 2021 • edited

codecov-io commented Apr 8, 2021

Codecov Report

bollwyvl commented Apr 8, 2021

bollwyvl commented Apr 9, 2021

bollwyvl commented Apr 9, 2021

bollwyvl commented Apr 12, 2021

fcollonval commented Apr 19, 2021

fcollonval Apr 19, 2021

Choose a reason for hiding this comment

bollwyvl Apr 19, 2021

Choose a reason for hiding this comment

fcollonval Apr 19, 2021

Choose a reason for hiding this comment

bollwyvl Apr 19, 2021

Choose a reason for hiding this comment

fcollonval Apr 19, 2021

Choose a reason for hiding this comment

bollwyvl commented Apr 19, 2021

bollwyvl commented Apr 8, 2021 •

edited