Rework ordering filter #1118

earshinov · 2019-09-02T17:21:41Z

earshinov · 2019-09-02T17:23:49Z

@carltongibson , @rpkilby , Could you take a look and share your thoughts?

I started with the code and tests.

The updated OrderingFilter maintains full compatibility with the old one. That is, it supports fields, field_labels and choices arguments as before. But it also accepts a new params argument in the form of { param name: ... } that allows to

order by several fields
order by Django expression(s)
apply inverse order (using F(...).desc() as was illustrated in the comment)

TODO:

document in docs/ref/filters.txt
update docs/guide/migration.txt

codecov-io · 2019-09-02T18:18:55Z

Codecov Report

Merging #1118 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   99.43%   99.45%   +0.02%     
==========================================
  Files          15       15              
  Lines        1235     1296      +61     
==========================================
+ Hits         1228     1289      +61     
  Misses          7        7

Impacted Files	Coverage Δ
django_filters/filters.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d5f2d...edda682. Read the comment docs.

al-muammar · 2019-12-21T23:02:04Z

@earshinov, any updates here? I would be happy to use nulls_last feature.

earshinov · 2019-12-23T18:34:17Z

@jihadik , Hi! No, I don't have any updates. I think I have pushed everything that I had, now awaiting feedback from django-filter maintainers. The version from this PR should already support nulls_last just fine. I would appreciate if you gave it a try and shared your feedback. It seems that the maintainers are not terribly interested in this feature, and additional user feedback could convince them to pay more attention to it, should they have time.

al-muammar

LGTM

al-muammar · 2019-12-23T23:14:39Z

django_filters/filters.py


-        self.param_map = {v: k for k, v in fields.items()}
+        self._params = params


Looking at the code around _params is going against the style. Just use self.params

Indeed, it seems that in the existing code underscores are only utilized to name backing fields used in the implementation of get/set properties. Renamed in edda682.

al-muammar · 2019-12-23T23:18:17Z

tests/models.py

-    first_name = SubCharField(max_length=100)
-    last_name = SubSubCharField(max_length=100)
+    first_name = SubCharField(max_length=100, null=True, blank=True)
+    last_name = SubSubCharField(max_length=100, null=True, blank=True)


I believe, by default an empty string is null. Also, you are not using forms, so blank=True is not needed. hence, you don't need to touch these two lines.

I believe, by default an empty string is null

I am not sure what you mean. null and empty string are different values and are treated differently both by Django ORM and the underlying database.

null=True is necessary because within tests I create some User instances with first_name=null to check handling of nulls. The default value for the null constructor parameter is False.

As for blank=True, maybe User instances are not tied to forms at the moment, but they can be in the future. I think it's a good practice to set blank=True once one defines null=True.

This is from the Django official doc: https://docs.djangoproject.com/en/3.0/ref/models/fields/#django.db.models.Field.null

As for the blank=True, I believe it should be added on demand. Otherwise, me reading the tests would scratch my head and think why is it necessary to have it here?

This is from the Django official doc: https://docs.djangoproject.com/en/3.0/ref/models/fields/#django.db.models.Field.null

Still, having null=False will prevent us from testing User(first_name=null)

As for the blank=True, I believe it should be added on demand. Otherwise, me reading the tests would scratch my head and think why is it necessary to have it here?

Well, I would rather scratch my head if I saw a field with null=True but blank=False. It clearly won't work with forms at all. I guess we need another opinion here, ideally from django-filter devs.

I have some experience with Django and would like to share my opinion on this matter. I think having blank=True, null=True on a django CharField is definitely not a best practice and even strongly disencouraged. It means you have two empty values in the database, NULL and '' (empty char). Therefore my recommendation is to rewrite the test by using a datetimeField (for example) with null=True, blank=True. In that case it makes logical sense and the correctness of the test is also faily easily determined.

Having said that I would also love to see this merged! Big applause for @earshinov for undertaking this project!

Hi @gabn88! Sorry that I didn't have time to reply during workdays. Thank you for your input, your suggestion about avoiding nullable/blank CharFields seems very reasonable. I reworked the tests using a DateTime model field instead of CharField and rebased the branch onto the latest master along the way. Hopefully these changes will help merge this PR.

al-muammar · 2019-12-23T23:23:18Z

@carltongibson, could I get attention for this PR?
@earshinov, it would be cool if you could merge the latest master, thanks!

earshinov · 2019-12-24T11:32:04Z

@jihadik , I rebased the PR on top of the master branch

…g behavior regarding nulls

Closes carltongibson#1021

…_eq__ operators

…el fields

…h the style of existing code

… of nulls

codecov-commenter · 2020-05-24T12:25:02Z

Codecov Report

Merging #1118 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   99.44%   99.46%   +0.02%     
==========================================
  Files          15       15              
  Lines        1255     1316      +61     
==========================================
+ Hits         1248     1309      +61     
  Misses          7        7

Impacted Files	Coverage Δ
django_filters/filters.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 737d75f...e192249. Read the comment docs.

forgoty · 2020-05-26T00:27:38Z

What is the progress about this task?
@rpkilby

jaap3 · 2022-11-09T09:45:08Z

Too bad this has gone stale, I'm in need of a way to have null values sorted to the bottom and had hoped django-filter supported it. Maybe some day...

axieum · 2022-11-17T12:48:56Z

I'm also keen to see this across the line 🙌

al-muammar reviewed Dec 23, 2019

View reviewed changes

earshinov force-pushed the rework-ordering-filter branch from af5dd33 to 33a500e Compare December 24, 2019 11:31

rpkilby mentioned this pull request Mar 4, 2020

Fix descending choices building for OrderingFilter #1176

Closed

rpkilby self-requested a review March 4, 2020 21:37

rpkilby mentioned this pull request Mar 5, 2020

Fix descending choices building for OrderingFilter #1183

Open

earshinov added 9 commits May 24, 2020 13:38

Add OrderingFilterTests.test_ordering_with_nulls to lock down orderin…

4c5e00e

…g behavior regarding nulls

Initial push for reworked OrderingFilter

5d781a2

Closes carltongibson#1021

Resolve errors reported by lint and isort

db1986f

Run isort

e21ba2d

Fix tests under Django 1.11 by defining custom F.__eq__ and OrderBy._…

25e130c

…_eq__ operators

Cover the case when ordering descriptor is specified as a list of mod…

36231d5

…el fields

Correct a comment

56f81cd

In the updated OrderingFilter class, rename _params to params to matc…

964fa26

…h the style of existing code

Remove if True which was probably used for debugging

ee0e6ba

earshinov force-pushed the rework-ordering-filter branch from edda682 to 20a6a66 Compare May 24, 2020 12:13

Use a nullable DateTime model field to test OrderingFilter's handling…

e192249

… of nulls

earshinov force-pushed the rework-ordering-filter branch from 20a6a66 to e192249 Compare May 24, 2020 12:22

Base automatically changed from master to main March 3, 2021 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework ordering filter #1118

Rework ordering filter #1118

earshinov commented Sep 2, 2019

earshinov commented Sep 2, 2019 •

edited

Loading

codecov-io commented Sep 2, 2019 •

edited

Loading

al-muammar commented Dec 21, 2019

earshinov commented Dec 23, 2019

al-muammar left a comment

al-muammar Dec 23, 2019

earshinov Dec 24, 2019

al-muammar Dec 23, 2019

earshinov Dec 24, 2019

al-muammar Dec 24, 2019 •

edited

Loading

earshinov Dec 24, 2019

gabn88 May 18, 2020

earshinov May 24, 2020

al-muammar commented Dec 23, 2019

earshinov commented Dec 24, 2019

codecov-commenter commented May 24, 2020 •

edited

Loading

forgoty commented May 26, 2020

jaap3 commented Nov 9, 2022

axieum commented Nov 17, 2022


		self.param_map = {v: k for k, v in fields.items()}
		self._params = params

Rework ordering filter #1118

Are you sure you want to change the base?

Rework ordering filter #1118

Conversation

earshinov commented Sep 2, 2019

earshinov commented Sep 2, 2019 • edited Loading

codecov-io commented Sep 2, 2019 • edited Loading

Codecov Report

al-muammar commented Dec 21, 2019

earshinov commented Dec 23, 2019

al-muammar left a comment

Choose a reason for hiding this comment

al-muammar Dec 23, 2019

Choose a reason for hiding this comment

earshinov Dec 24, 2019

Choose a reason for hiding this comment

al-muammar Dec 23, 2019

Choose a reason for hiding this comment

earshinov Dec 24, 2019

Choose a reason for hiding this comment

al-muammar Dec 24, 2019 • edited Loading

Choose a reason for hiding this comment

earshinov Dec 24, 2019

Choose a reason for hiding this comment

gabn88 May 18, 2020

Choose a reason for hiding this comment

earshinov May 24, 2020

Choose a reason for hiding this comment

al-muammar commented Dec 23, 2019

earshinov commented Dec 24, 2019

codecov-commenter commented May 24, 2020 • edited Loading

Codecov Report

forgoty commented May 26, 2020

jaap3 commented Nov 9, 2022

axieum commented Nov 17, 2022

earshinov commented Sep 2, 2019 •

edited

Loading

codecov-io commented Sep 2, 2019 •

edited

Loading

al-muammar Dec 24, 2019 •

edited

Loading

codecov-commenter commented May 24, 2020 •

edited

Loading