Use the new IPython completer API #222

Merged
merged 1 commit into from May 30, 2017

Conversation

Projects
None yet
5 participants
@Carreau
Member

Carreau commented Jan 31, 2017

Work in progress, I want to see if I can use the metadata fields for frontends to opt-in to some extra informations.

Also how backward compatible would it be to have a metadata field on the complete_request ? My idea is for testing would have been to have an "Accept:" like field for the kernel to know whether to compute the new completions.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Feb 1, 2017

Member

Also how backward compatible would it be to have a metadata field on the complete_request ?

Adding a field should be a minor revision, and if things work when that field is empty / missing, it's not a compatibility problem at all.

Member

minrk commented Feb 1, 2017

Also how backward compatible would it be to have a metadata field on the complete_request ?

Adding a field should be a minor revision, and if things work when that field is empty / missing, it's not a compatibility problem at all.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Feb 1, 2017

Member

I'm ok with introducing new fields rather than metadata for the final version of this (that can be ignored by some frontends), effectively matches and enhanced_matches.

The annoying thing I've noticed about our "metadata" fields is that people come to rely on the metadata if it gets merged in as a metadata field, and then it's no longer extraneous metadata -- it's an expected format with it's own versioning.

Member

rgbkrk commented Feb 1, 2017

I'm ok with introducing new fields rather than metadata for the final version of this (that can be ignored by some frontends), effectively matches and enhanced_matches.

The annoying thing I've noticed about our "metadata" fields is that people come to rely on the metadata if it gets merged in as a metadata field, and then it's no longer extraneous metadata -- it's an expected format with it's own versioning.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 1, 2017

Member

I'm ok with introducing new fields rather than metadata for the final version of this (that can be ignored by some frontends), effectively matches and enhanced_matches.

The annoying thing I've noticed about our "metadata" fields is that people come to rely on the metadata if it gets merged in as a metadata field, and then it's no longer extraneous metadata -- it's an expected format with it's own versioning.

Agreed, but right now adding things into the metadata field allow us to experiment without changing the wireformat and having to deal with the serialisation layer.

Once we get the fields right it's "easy" to make an enhancement proposal to move them up one level.

I've push a commit that send these new informations in the metadata field for now.
We can start bikeshedding on names and necessary fields.

Member

Carreau commented Feb 1, 2017

I'm ok with introducing new fields rather than metadata for the final version of this (that can be ignored by some frontends), effectively matches and enhanced_matches.

The annoying thing I've noticed about our "metadata" fields is that people come to rely on the metadata if it gets merged in as a metadata field, and then it's no longer extraneous metadata -- it's an expected format with it's own versioning.

Agreed, but right now adding things into the metadata field allow us to experiment without changing the wireformat and having to deal with the serialisation layer.

Once we get the fields right it's "easy" to make an enhancement proposal to move them up one level.

I've push a commit that send these new informations in the metadata field for now.
We can start bikeshedding on names and necessary fields.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Feb 1, 2017

Member

Sounds great, thanks Matthias.

Member

rgbkrk commented Feb 1, 2017

Sounds great, thanks Matthias.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Feb 2, 2017

Member

Sounds like a good plan.

Member

minrk commented Feb 2, 2017

Sounds like a good plan.

@minrk minrk changed the title from [WIP] Use the new IPython completer API. to [WIP] Use the new IPython completer API Feb 7, 2017

@Carreau Carreau referenced this pull request in nteract/nteract Apr 17, 2017

Merged

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

2 of 2 tasks complete

@Carreau Carreau referenced this pull request in jupyter/qtconsole Apr 25, 2017

Closed

Jedi-based autocompletion does not work in QtConsole #210

ipykernel/ipkernel.py
+ end=comp.end,
+ text=comp.text,
+ type=comp.type,
+ ))

This comment has been minimized.

@ellisonbg

ellisonbg May 30, 2017

Member

Dedent these )) to the level of comps.

@ellisonbg

ellisonbg May 30, 2017

Member

Dedent these )) to the level of comps.

ipykernel/ipkernel.py
+ ))
+
+ if completions:
+ s = completions[0].start

This comment has been minimized.

@ellisonbg

ellisonbg May 30, 2017

Member

Seems reasonable to use completions[0] but might there ever be a reason to not pick this one?

@ellisonbg

ellisonbg May 30, 2017

Member

Seems reasonable to use completions[0] but might there ever be a reason to not pick this one?

This comment has been minimized.

@Carreau

Carreau May 30, 2017

Member

No. rectify_completion is made to have all completions have the same beginning and end and pad these potentially ugly characters. I can re-assert that len(set(c.start for c in completions)) == 1, but that seem unnecessary.

@Carreau

Carreau May 30, 2017

Member

No. rectify_completion is made to have all completions have the same beginning and end and pad these potentially ugly characters. I can re-assert that len(set(c.start for c in completions)) == 1, but that seem unnecessary.

This comment has been minimized.

@ellisonbg

ellisonbg May 30, 2017

Member

Sounds good!

@ellisonbg

ellisonbg May 30, 2017

Member

Sounds good!

ipykernel/ipkernel.py
+ return {'matches': matches,
+ 'cursor_end': e,
+ 'cursor_start': s,
+ 'metadata': {'_experimental': comps},

This comment has been minimized.

@ellisonbg

ellisonbg May 30, 2017

Member

The only big question is how to encode this in the message data. I think this approach of putting this into the metadata makes sense. Maybe add a suffix to help a frontend to distinguish if there are multiple experimental extensions:

_experimental_type_data

@ellisonbg

ellisonbg May 30, 2017

Member

The only big question is how to encode this in the message data. I think this approach of putting this into the metadata makes sense. Maybe add a suffix to help a frontend to distinguish if there are multiple experimental extensions:

_experimental_type_data

This comment has been minimized.

@ellisonbg

ellisonbg May 30, 2017

Member

Some ideas:

  • _jupyter_types_prototype
  • _jupyter_types_experimental
@ellisonbg

ellisonbg May 30, 2017

Member

Some ideas:

  • _jupyter_types_prototype
  • _jupyter_types_experimental

@Carreau Carreau changed the title from [WIP] Use the new IPython completer API to Use the new IPython completer API May 30, 2017

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau May 30, 2017

Member

Rebased and comment taken into account.
This will likely also affect a bit completions in the normal notebooks, it there are bugs, please let me know.

Member

Carreau commented May 30, 2017

Rebased and comment taken into account.
This will likely also affect a bit completions in the normal notebooks, it there are bugs, please let me know.

@ellisonbg ellisonbg merged commit 8acaee8 into ipython:master May 30, 2017

4 checks passed

codecov/patch 84% of diff hit (target 0%)
Details
codecov/project 70.68% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau May 30, 2017

Member
Member

Carreau commented May 30, 2017

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk May 30, 2017

Member

😮

🎉 🎉 🎉

Member

rgbkrk commented May 30, 2017

😮

🎉 🎉 🎉

@lgeiger lgeiger referenced this pull request in nteract/hydrogen May 30, 2017

Merged

Try out new IPython completer API #826

@Carreau Carreau deleted the Carreau:new-completer branch May 31, 2017

@takluyver takluyver added this to the 4.7 milestone Aug 2, 2017

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