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

Enabling sort_attribute #138

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

luord
Copy link
Contributor

@luord luord commented Jul 16, 2018

In a project I did recently, I needed to change the default sort, I saw that this attribute existed in the Meta class for ModelResource, but wasn't being used.

This PR is an attempt of enabling it for usage.

@luord
Copy link
Contributor Author

luord commented Jul 24, 2018

Hadn't noticed the conflicts, updated now with the conflicts fixed.

@luord
Copy link
Contributor Author

luord commented Dec 6, 2018

Merged the reversion from the other branch.

Don't know if this will ever be merged but, for what it's worth, I think the sort_attribute should be passed as an argument and not decided exclusively from the meta in each manager.

This way loosens coupling, IMO, and the sort could work for any manager and not just sqlalchemy, plus it's more consistent on what the sort is expected to be. Namely a tuple of the field, its name and reverse.

@coveralls
Copy link

coveralls commented Dec 8, 2018

Coverage Status

Coverage increased (+0.06%) to 89.172% when pulling 22a5d64 on luord:default-sort into 66f11dc on biosustain:master.

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.

3 participants