8000 Removed unreachable condition in TruncBase.convert_value() by timgraham · Pull Request #19461 · django/django · GitHub
[go: up one dir, main page]
Skip to content

Removed unreachable condition in TruncBase.convert_value() #19461

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 27, 2025
+1 −3
Merged

Removed unreachable condition in TruncBase.convert_value() #19461

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions django/db/models/functions/datetime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,7 @@ def convert_value(self, value, expression, connection):
"zone definitions for your database installed?"
)
elif isinstance(value, datetime):
if value is None:
pass
elif isinstance(self.output_field, DateField):
if isinstance(self.output_field, DateField):
value = value.date()
elif isinstance(self.output_field, TimeField):
value = value.time()
Comment on lines 357 to 361
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this whole branch is uncovered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the database, I think. This may merit further investigation to clarify, but at least the first branch is run on PostgreSQL: https://djangoci.com/job/django-coverage-postgis/HTML_20Coverage_20Report/z_710479c84428724c_datetime_py.html#t357

I needed to customize the entire method on MongoDB, so I'm thinking about how I can add some hook in order to remove that monkeypatch in a future version of Django.
mongodb/django-mongodb-backend@85f02ef#diff-9bb9fc083062f41644e45753aef34a4c8381d250fed38ad42b0332ef39041f23R203-R227

Copy link
Member
@charettes charettes May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks didn't know we had this coverage job as well.

So it's not run on SQLite because (datetime|date|time)_trunc_sql run specialized Python functions that already return the proper type.

The first branch is implicitly tested by these usage of the __date transform in aggregation tests

django/tests/aggregation/tests.py

Line 2216 in 825ddda

"original_opening__date",

But we don't appear to have the equivalent coverage for __time

Expand Down
0