-
Notifications
You must be signed in to change notification settings - Fork 60
feat: Sort atlas listings alphabetically and add corresponding tests (#612) #688
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
base: main
Are you sure you want to change the base?
Conversation
…inglobe#612 where updated the atlas listing functions to return results in alphabetical order instead of chronological downloaded order
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Hi! This is my first contribution to Brainglobe 😊 The CI is waiting for workflow approval since this is a first-time contribution. |
|
Thanks @Vedant936-ai. Someone will review it when there is time. In the meantime, could I ask why the docstrings were removed? |
Oh , about that the docstring got removed because the pre-commit auto-fix changed it when I tried to fix the D200 lint error. I will insert it again |
|
Hi @Vedant936-ai, are you able to get back to this? No worries either way. |
Hey @adamltyson . Yes the docstrings are added . Actually it was added way before but conflicts were yet to be resolved . Now I all the conflicts are resolved. And it's proper now , you may check it . |
alessandrofelder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Vedant936-ai
Code changes look reasonable to me, and I did some manual testing successfully.
It would be good to take this opportunity to write some tests in https://github.com/brainglobe/brainglobe-atlasapi/blob/main/tests/atlasapi/test_list_atlases.py that check the alphabetical order is returned by each of the functions we've modified here.
Let us know if you'd be willing to add this to the PR (I will be happy either way) - thanks!
Hi @alessandrofelder Sure , I will add the tests for alphabetical order fn in the file and will commit to this PR only . Thankyou |
for more information, see https://pre-commit.ci
|
@alessandrofelder I have added the tests , https://github.com/brainglobe/brainglobe-atlasapi/pull/688/files#diff-3e4cc0bff581e3d9373a5544a271a5209092e34ce086b69720ae59b316716ae9 |
Description
What is this PR?
This pull request updates the atlas listing functions so that they return atlas names in alphabetical order. This improves clarity, user experience, and consistency across the API. The change addresses Issue #612.
Why is this PR needed?
Currently, the functions return atlas names based on the chronological order in which they were downloaded. This ordering can be confusing for users and makes it harder to locate specific atlases. Alphabetical ordering provides a predictable and user-friendly structure.
What does this PR do?
References
This PR directly addresses and resolves Issue #612.
How has this PR been tested?
Is this a breaking change?
This is not a breaking change. It only modifies the order in which results are returned, without altering the underlying data structures or API behavior.
Does this PR require documentation updates?
No documentation updates are required as the functionality of the API remains unchanged. Only the ordering of results is improved.
Checklist