8000 Rename convert to _convert, as it is a private function by sciunto · Pull Request #4590 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Rename convert to _convert, as it is a private function #4590

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 26, 2020
Merged

Rename convert to _convert, as it is a private function #4590

merged 1 commit into from
Apr 26, 2020

Conversation

sciunto
Copy link
Member
@sciunto sciunto commented Apr 17, 2020

Description

I do not understand why convert, which is not a private function, is not exposed in the API.

I'm opened to discussion with this PR.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@emmanuelle
Copy link
Member

It would be logical to expose this function indeed. That said, its behaviour will probably change quite a lot when we drop the range rescaling in version 1.0, so I'm tempted to wait until then so that it's one less function to deprecate...

@jni
Copy link
Member
jni commented Apr 19, 2020

I agree with @emmanuelle, I would rather leave this semi-private for now. I would be more inclined to merge a PR renaming it to _convert.

@jni jni mentioned this pull request Apr 20, 2020
Closed
@sciunto sciunto changed the title Add convert to the API Rename convert to _convert, as it is a private function Apr 21, 2020
@sciunto
Copy link
Member Author
sciunto commented Apr 21, 2020

Hopefully, it passes the tests.

emmanuelle
emmanuelle approved these changes Apr 26, 2020
View reviewed changes
Copy link
Member
@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @sciunto ! One more approval for this PR please @scikit-image/core !

@emmanuelle emmanuelle merged commit 49361aa into scikit-image:master Apr 26, 2020
@hmaarrfk
Copy link
Member

It isn't fun to have these decisions be made without deprecations.

I would like to make a PR for a deprecation cycle to be entered.

convert is very useful when trying to convert between dtypes programatically.

@emmanuelle
Copy link
Member

@hmaarrfk the only reason a deprecation cycle was not used is because the function was not in the public API...

@hmaarrfk hmaarrfk mentioned this pull request May 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanv stefanv stefanv approved these changes

@emmanuelle emmanuelle emmanuelle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sciunto @emmanuelle @jni @hmaarrfk @stefanv
0