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

Update changelog for 4.7 release #255

Merged
merged 1 commit into from Aug 8, 2017
Merged

Conversation

takluyver
Copy link
Member

@takluyver takluyver added this to the 4.7 milestone Aug 2, 2017
@takluyver
Copy link
Member Author

takluyver commented Aug 2, 2017

@Carreau One of the merged PRs for this release is to 'use the new IPython completer API' (#222). We probably need ipykernel to depend on a newer version of IPython for that (it currently requires >= 4.0). Do you know what version of IPython it now needs?

@takluyver
Copy link
Member Author

Oh, looking closer I see that it still falls back to the old mechanism if it can't import the new one.

But should the fact that it's importing provisionalcompleter be a red flag? Presumably that's not the name we're exposing as a public API.

@minrk
Copy link
Member

minrk commented Aug 2, 2017

But should the fact that it's importing provisionalcompleter be a red flag?

Seems like yes. Importing this suggests that provisionalcompleter is a semi-public API of IPython that won't change. Is that true? If it's not and the upstream API is indeed provisional, I think we need error handling other than ImportError, and possibly a manual disable flag.

@Carreau
Copy link
Member

Carreau commented Aug 2, 2017

provisionalcompleter API wont' change, it's just a context manager that make any uses of other semi-private API not raise when inside of the context manager. I can change ipykernel to not re-expose these.

Basically

self.shell.Completer.completions(code, cursor_pos) # raises
with provisionalcompleter():
    self.shell.Completer.completions(code, cursor_pos) # does not raise. 

@Carreau
Copy link
Member

Carreau commented Aug 2, 2017

I'll make a PR with a disable flag, but I don't want to ignore errors or we wont' get potential bug reports.

@takluyver
Copy link
Member Author

If we're satisfied that it won't change, should we call it something else? I understand 'provisional' to mean 'this might change'.

I'm not too worried about accidentally exposing it from ipykernel, my concern is more that it looks like ipykernel is using a new not-really-public API from IPython.

@Carreau
Copy link
Member

Carreau commented Aug 2, 2017

The not really stable API is self.shell.Completer.completions() (and a couple of others). These few will likely change, but I don't want to have a leading underscore to replace everywhere later on. Hence I made it raise by default and expose provisionalcompleter context manager to be clear that you use non stable API inside at your own risk.

So provisionalcompleter is kinda public in the sens that if you use it you've read it's docstring, and are using unstable API willingly:

.. note:: Unstable

        By using this context manager you agree that the API in use may change
        without warning, and that you won't complain if they do so.

        You also understand that if the API is not to you liking you should report
        a bug to explain your use case upstream and improve the API and will loose
        credibility if you complain after the API is make stable.

        We'll be happy to get your feedback , feature request and improvement on
        any of the unstable APIs !

@takluyver
Copy link
Member Author

@minrk Are you OK with that for the 4.7 release (once #257 is merged)?

@minrk
Copy link
Member

minrk commented Aug 4, 2017

@takluyver yup! go for it. I still don't fully understand what's provisional about the provisional APIs, but 👍 to shipping what we have.

@minrk minrk merged commit a243dac into ipython:master Aug 8, 2017
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