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

Make functions consistent for filtering, sorting and pagination #17

Open
tomekowal opened this issue Sep 12, 2019 · 3 comments
Open

Make functions consistent for filtering, sorting and pagination #17

tomekowal opened this issue Sep 12, 2019 · 3 comments

Comments

@tomekowal
Copy link
Contributor

While writing documentation, I found that the functions we use are inconsistent.

filter sort pagination
put_filter put_sort put_pagination 👍
clear_sort 👎
remove_sort 👎
add_sort 👎
merge_default_sort 👎
put_default_filters put_default_sort put_default_pagination 👍
put_filter_function put_sort_function NOT NEEED 👍
remove_filter_function 👎

I think we can safely delete those with 👎 That "flexibility" in sorting is confusing.

@dimitarvp
Copy link
Contributor

  • I can see us removing add_sort in favour of put_sort, agreed.
  • clear_sort can probably go as well, although I am not sure it does harm by staying in.
  • merge_default_sort was kind of a bandaid for replacing our Collection. 50/50 on removing this though; we might end up reimplementing this function in CIO if we remove it from here.

But do you really think it makes sense to kill the ability to remove sorting clauses? I can very easily see people who would build forms with our library and would like to just uncheck a checkbox somewhere and stop sorting on 2 criteria and only go with 1. We'd immediately kill off that use case. And it's not a rare one.

As for the filter functions, I am not in the loop of QB for a while. Maybe your recent changes made some of the *_filter_function functions obsolete?

@tomekowal
Copy link
Contributor Author

Filter functions are all OK :) I believe unchecking the field doesn't translate to using clear_sort. Basically next request builds new QB from scratch, so all "negative" functions like clear and remove can go.

I'll check the difference between put_default_sort and merge_default_sort.

@dimitarvp
Copy link
Contributor

Mish-mash of notes:

  • put_sort name is misleading. What it does is it replaces all sort directives. Comparing to put_filters (note the plural) we should likely rename put_sort to put_sort_directives.
  • Maybe we should rename add_sort to append_sort.
  • Globally replace put_ with replace_.
  • Globally replace these:
    • *_filter with *_filter_params.
    • *_sort with *_sort_params.
    • *_pagination with *_pagination_params.
  • Add test to make sure that append_sort (or add_sort) makes sure not to duplicate the same sort clause.

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

No branches or pull requests

2 participants