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

Add function alphanumeric_key in __all__ of io.collection.py #4321

Closed
wants to merge 1 commit into from

Conversation

sciunto
Copy link
Member

@sciunto sciunto commented Nov 25, 2019

Description

Fixes #4309

Test:

In [3]: skimage.lookfor('alphanu' 
   ...: )                                                                                                                                                                                                                                    
Search results for 'alphanu'
----------------------------
skimage.io.alphanumeric_key
    Convert string to list of strings and ints that gives intuitive sorting.
skimage.io.ImageCollection
    Load and manage a collection of image files.

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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@sciunto sciunto added the 📄 type: Documentation Updates, fixes and additions to documentation label Nov 25, 2019
@sciunto sciunto added this to the 0.17 milestone Nov 25, 2019
@sciunto sciunto requested a review from jni November 25, 2019 07:04
@soupault
Copy link
Member

soupault commented Nov 27, 2019

Frankly speaking, I am not sure if this is a good addition to the public API, since the function is rather simple and covers only quite specific naming case. Perhaps, we could have more core members to share their opinions.

@jni
Copy link
Member

jni commented Nov 28, 2019

well, we wanted to use it in at least one other place, namely napari. I also don't think it warrants its own package or anything (hello left-pad), and it is complex enough that I don't want to reimplement it everywhere... But I agree that it's kinda weird to provide it. I just don't think there is a better option.

@hmaarrfk
Copy link
Member

https://pypi.org/project/natsort/ ???

@jni
Copy link
Member

jni commented Nov 28, 2019

@hmaarrfk that's pretty nice, thanks!

@sciunto
Copy link
Member Author

sciunto commented Dec 2, 2019

Should I close this? Should we remove this function?

@jni
Copy link
Member

jni commented Dec 2, 2019

I would not remove it for sure — no chance we want that dependency. The only question is whether we want it to be part of the public API. Of that I’m not sure. I suggest we wait for more devs to weigh in.

@sciunto
Copy link
Member Author

sciunto commented Dec 2, 2019

I would be in favor to add the link to natsort in the dosctring as an indication.

@stefanv
Copy link
Member

stefanv commented Dec 6, 2019

Err, woops, just left a comment on the wrong issue. Let's try again :)

@jni I'd suggest renaming this function to better explain what it does, as well as moving it from io to util.

@lagru
Copy link
Member

lagru commented Dec 6, 2019

How about as_alphanumeric_key or as_natural_sort_key? I like the latter.

@emmanuelle
Copy link
Member

This one is a bit tricky, maybe we should take the opportunity of a dev meeting to discuss the best name.

@emmanuelle
Copy link
Member

As discussed in the Atlantic meeting Jan 31st, let's keep this function private and add an example with natsort in the documentation.

@jni
Copy link
Member

jni commented Feb 4, 2020

Cool, sounds good to me.

@jni jni closed this Feb 4, 2020
@sciunto sciunto deleted the fix_all_in_io branch March 17, 2020 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

io documentation is incomplete
7 participants