ux(editor): Poking around completer API for custom rendering #1650

Merged
merged 1 commit into from Jun 1, 2017

Conversation

Projects
None yet
4 participants
@Carreau
Member

Carreau commented Apr 14, 2017

Just Poking around to see how to get our custom rendering function.
It's a bit dumb as you need to pass the render function for each
completion item and not to as a global render function.

I guess that could be patched in codemirror upstream, but let's do this
for now.

From the UI-side of things it should not change much, but that should
allow to test new completer UI for the extra information that can be
returned from ipykernel (that needs patches).

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Apr 17, 2017

Codecov Report

Merging #1650 into master will decrease coverage by 0.55%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
- Coverage   79.04%   78.48%   -0.56%     
==========================================
  Files          62       62              
  Lines        2047     2064      +17     
==========================================
+ Hits         1618     1620       +2     
- Misses        429      444      +15

codecov-io commented Apr 17, 2017

Codecov Report

Merging #1650 into master will decrease coverage by 0.55%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
- Coverage   79.04%   78.48%   -0.56%     
==========================================
  Files          62       62              
  Lines        2047     2064      +17     
==========================================
+ Hits         1618     1620       +2     
- Misses        429      444      +15
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 17, 2017

Member

So far, with a custom ipykernel branch I can get the following:

screen shot 2017-04-16 at 20 46 56

Member

Carreau commented Apr 17, 2017

So far, with a custom ipykernel branch I can get the following:

screen shot 2017-04-16 at 20 46 56

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 17, 2017

Member

Todo, hover on the left-hand-side should display a tooltip of the meaning.

Member

Carreau commented Apr 17, 2017

Todo, hover on the left-hand-side should display a tooltip of the meaning.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 17, 2017

Member

This is so cool!

Member

rgbkrk commented Apr 17, 2017

This is so cool!

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 17, 2017

Member

It would be good to get a design pass at this.
It is not necessary as it triggers only with my fork of ipykernel, but it would be great.
If someone knows the types of completions provided with Atom, we could copy that as well.

The ipykernel is not stable, so if there are things you want to add now is still the time.
is @lgeiger the right person to ping ? Especially since Hydrogen can make use of these hints as well.

Member

Carreau commented Apr 17, 2017

It would be good to get a design pass at this.
It is not necessary as it triggers only with my fork of ipykernel, but it would be great.
If someone knows the types of completions provided with Atom, we could copy that as well.

The ipykernel is not stable, so if there are things you want to add now is still the time.
is @lgeiger the right person to ping ? Especially since Hydrogen can make use of these hints as well.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 17, 2017

Member

please try with ipython/ipykernel#222

Member

Carreau commented Apr 17, 2017

please try with ipython/ipykernel#222

@lgeiger

This comment has been minimized.

Show comment
Hide comment
@lgeiger

lgeiger Apr 17, 2017

Member

@Carreau Excellent work 🎉 ! Great that jupyter/jupyter_client#51 is picked up.

Atom's autocomplete API is definitely a awesome source of inspiration since it has support for a lot of additional metadata.
Apart from the custom icons depending on the completion type, I really enjoy seeing the function signature and a peak at the docs string.

There's a rather old branch on Hydrogen using ipython/ipython#8720 to try out support for completion metadata.
I'm more than happy to collaborate on supporting this in Hydrogen. It's probably a good place to try out more metadata since it's quite easy to implement thanks to Atom's autocomplete API.

Member

lgeiger commented Apr 17, 2017

@Carreau Excellent work 🎉 ! Great that jupyter/jupyter_client#51 is picked up.

Atom's autocomplete API is definitely a awesome source of inspiration since it has support for a lot of additional metadata.
Apart from the custom icons depending on the completion type, I really enjoy seeing the function signature and a peak at the docs string.

There's a rather old branch on Hydrogen using ipython/ipython#8720 to try out support for completion metadata.
I'm more than happy to collaborate on supporting this in Hydrogen. It's probably a good place to try out more metadata since it's quite easy to implement thanks to Atom's autocomplete API.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 22, 2017

Member

I'm confused as of Why the tests are failing. It complains about React props which I haven't touched (AFAIU)

Member

Carreau commented Apr 22, 2017

I'm confused as of Why the tests are failing. It complains about React props which I haven't touched (AFAIU)

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 23, 2017

Member

Looks like one of our dependencies had an in-place change. I'll be spending some time this week bringing us up to date, then you can rebase. All set!

Member

rgbkrk commented Apr 23, 2017

Looks like one of our dependencies had an in-place change. I'll be spending some time this week bringing us up to date, then you can rebase. All set!

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau May 5, 2017

Member

rebased. The rebase was clean and seem to work.

Member

Carreau commented May 5, 2017

rebased. The rebase was clean and seem to work.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk May 5, 2017

Member

Sweet.

The Travis error I'm seeing looks related here, perhaps just a missing property in the tests?

  ● complete › handles code completion
    TypeError: Cannot read property '_experimental' of undefined
      
      at MapSubscriber.project (packages/editor/src/complete.js:115:191)
      at MapSubscriber.Object.<anonymous>.MapSubscriber._next (node_modules/rxjs/operator/map.js:77:35)
      at MapSubscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at FirstSubscriber.Object.<anonymous>.FirstSubscriber._emitFinal (node_modules/rxjs/operator/first.js:135:25)
      at FirstSubscriber.Object.<anonymous>.FirstSubscriber._emit (node_modules/rxjs/operator/first.js:118:14)
      at FirstSubscriber.Object.<anonymous>.FirstSubscriber._next (node_modules/rxjs/operator/first.js:97:18)
      at FirstSubscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at MapSubscriber.Object.<anonymous>.MapSubscriber._next (node_modules/rxjs/operator/map.js:83:26)
      at MapSubscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at SafeSubscriber._next (packages/editor/node_modules/@nteract/messaging/lib/index.js:105:20)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.__tryOrUnsub (node_modules/rxjs/Subscriber.js:236:16)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.next (node_modules/rxjs/Subscriber.js:185:22)
      at Subscriber.Object.<anonymous>.Subscriber._next (node_modules/rxjs/Subscriber.js:125:26)
      at Subscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at SafeSubscriber._next (packages/editor/node_modules/@nteract/messaging/lib/index.js:70:20)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.__tryOrUnsub (node_modules/rxjs/Subscriber.js:236:16)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.next (node_modules/rxjs/Subscriber.js:185:22)
      at Subscriber.Object.<anonymous>.Subscriber._next (node_modules/rxjs/Subscriber.js:125:26)
      at Subscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at Subject.Object.<anonymous>.Subject.next (node_modules/rxjs/Subject.js:55:25)
      at Object.<anonymous> (packages/editor/__tests__/editor-spec.js:115:14)
Member

rgbkrk commented May 5, 2017

Sweet.

The Travis error I'm seeing looks related here, perhaps just a missing property in the tests?

  ● complete › handles code completion
    TypeError: Cannot read property '_experimental' of undefined
      
      at MapSubscriber.project (packages/editor/src/complete.js:115:191)
      at MapSubscriber.Object.<anonymous>.MapSubscriber._next (node_modules/rxjs/operator/map.js:77:35)
      at MapSubscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at FirstSubscriber.Object.<anonymous>.FirstSubscriber._emitFinal (node_modules/rxjs/operator/first.js:135:25)
      at FirstSubscriber.Object.<anonymous>.FirstSubscriber._emit (node_modules/rxjs/operator/first.js:118:14)
      at FirstSubscriber.Object.<anonymous>.FirstSubscriber._next (node_modules/rxjs/operator/first.js:97:18)
      at FirstSubscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at MapSubscriber.Object.<anonymous>.MapSubscriber._next (node_modules/rxjs/operator/map.js:83:26)
      at MapSubscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at SafeSubscriber._next (packages/editor/node_modules/@nteract/messaging/lib/index.js:105:20)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.__tryOrUnsub (node_modules/rxjs/Subscriber.js:236:16)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.next (node_modules/rxjs/Subscriber.js:185:22)
      at Subscriber.Object.<anonymous>.Subscriber._next (node_modules/rxjs/Subscriber.js:125:26)
      at Subscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at SafeSubscriber._next (packages/editor/node_modules/@nteract/messaging/lib/index.js:70:20)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.__tryOrUnsub (node_modules/rxjs/Subscriber.js:236:16)
      at SafeSubscriber.Object.<anonymous>.SafeSubscriber.next (node_modules/rxjs/Subscriber.js:185:22)
      at Subscriber.Object.<anonymous>.Subscriber._next (node_modules/rxjs/Subscriber.js:125:26)
      at Subscriber.Object.<anonymous>.Subscriber.next (node_modules/rxjs/Subscriber.js:89:18)
      at Subject.Object.<anonymous>.Subject.next (node_modules/rxjs/Subject.js:55:25)
      at Object.<anonymous> (packages/editor/__tests__/editor-spec.js:115:14)
@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk May 5, 2017

Member

Wow!

screen shot 2017-05-05 at 2 44 01 pm

Looking pretty slick. 😄

Member

rgbkrk commented May 5, 2017

Wow!

screen shot 2017-05-05 at 2 44 01 pm

Looking pretty slick. 😄

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau May 5, 2017

Member

How do I fix the tests, codemirror either expect a list of string, or a list of object that have callbacks.

How to I expect() something which is not json ? Do I drop the callbacks manually? can I expect a either/or ?

Member

Carreau commented May 5, 2017

How do I fix the tests, codemirror either expect a list of string, or a list of object that have callbacks.

How to I expect() something which is not json ? Do I drop the callbacks manually? can I expect a either/or ?

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk May 6, 2017

Member

You can do checks on individual elements instead of the whole object.

Member

rgbkrk commented May 6, 2017

You can do checks on individual elements instead of the whole object.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau May 6, 2017

Member

Also I just got IPython to show the function signature as well, I'm going to think about how to show that in the completer as well. What do you think? just push it in the displayed text ?

You can do checks on individual elements instead of the whole object.

Duh. Yes, you are right.

Member

Carreau commented May 6, 2017

Also I just got IPython to show the function signature as well, I'm going to think about how to show that in the completer as well. What do you think? just push it in the displayed text ?

You can do checks on individual elements instead of the whole object.

Duh. Yes, you are right.

ux(editor): Poking around completer API for custom rendering
Just Poking around to see how to get our custom rendering function.
It's a bit dumb as you need to pass the render function for each
completion item and not to as a global render function.

I guess that could be patched in codemirror upstream, but let's do this
for now.

From the UI-side of things it should not change much, but that should
allow to test new completer UI for the extra information that can be
returned from ipykernel (that needs patches).

documentation and style

Handle metadata unset.

@Carreau Carreau changed the title from [WIP] ux(editor): Poking around completer API for custom rendering to ux(editor): Poking around completer API for custom rendering Jun 1, 2017

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jun 1, 2017

Member

Travis is happy, it work on ipykernel master.

Member

Carreau commented Jun 1, 2017

Travis is happy, it work on ipykernel master.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Jun 1, 2017

Member

Yeah, I think we'll handle the coverage drop for now.

Member

rgbkrk commented Jun 1, 2017

Yeah, I think we'll handle the coverage drop for now.

@rgbkrk rgbkrk merged commit e4e56e9 into nteract:master Jun 1, 2017

2 of 4 checks passed

codecov/patch 27.77% of diff hit (target 30%)
Details
codecov/project 78.48% (-0.56%) compared to 0eb6ca3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Jun 1, 2017

Member

Guess this means we've got nteract and hydrogen support ready to go!

Member

rgbkrk commented Jun 1, 2017

Guess this means we've got nteract and hydrogen support ready to go!

@akabe akabe referenced this pull request in akabe/ocaml-jupyter Aug 22, 2017

Closed

Show types for code completion #47

@akabe akabe referenced this pull request in akabe/jupyter-completion-hint Sep 10, 2017

Open

Cannot work for Firefox and Safari #2

@akabe akabe referenced this pull request in akabe/ocaml-jupyter Dec 1, 2017

Closed

Completion hints #48

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot Apr 2, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

lock bot commented Apr 2, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@nteract nteract locked and limited conversation to collaborators Apr 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.