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

refactor: update all_day detection logic #2601

Merged

Conversation

miketheman
Copy link
Member

Description

Instead of using a particular resolution on an object, which differs between datetime.date and datetime.datetime objects, operate on parent class of datetime and convert all dt to timezone-aware datetime values.

This is also in accordance to the model field being a DateTimeField, so we should always be passing the correctly-created object, instead of a datetime.date(), raising received a naive datetime warnings.

Removes unused constants.

Closes

Closes #2415


Other notes, for fun.

The warnings stem from this little bitty function:

def extract_date_or_datetime(dt):
if isinstance(dt, datetime.datetime):
return convert_dt_to_aware(dt)
return dt

Since the ICS importer is often met with a string that looks like a date-only, like this one:

DTSTART;VALUE=DATE:20160402

the returned type is datetime.date, not a datetime.datetime.

The change in events/utils.py drops the warnings, but fails two test cases:

======================================================================
FAIL: test_import_event_excludes_ending_day_when_all_day_is_true (events.tests.test_importer.EventsImporterTestCase.test_import_event_excludes_ending_day_when_all_day_is_true)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/code/events/tests/test_importer.py", line 131, in test_import_event_excludes_ending_day_when_all_day_is_true
    self.assertTrue(all_day_event.next_or_previous_time.all_day)
AssertionError: False is not true

======================================================================
FAIL: test_modified_event (events.tests.test_importer.EventsImporterTestCase.test_modified_event)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/code/events/tests/test_importer.py", line 63, in test_modified_event
    self.assertTrue(e.next_or_previous_time.all_day)
AssertionError: False is not true

These come from the all_day calculation over here:

# Let's mark those occurrences as 'all-day'.
all_day = (
dt_start.resolution == DATE_RESOLUTION or
dt_end.resolution == DATE_RESOLUTION
)

which has now been updated.

Instead of using a particular resolution on an object, which differs
between `datetime.date` and `datetime.datetime` objects, operate on
parent class of `datetime` and convert all `dt` to timezone-aware
`datetime` values.

This is also in accordance to the model field being a `DateTimeField`,
so we should always be passing the correctly-created object, instead of
a `datetime.date()`, raising `received a naive datetime` warnings.

Removes unused constants.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@ewdurbin ewdurbin merged commit 00b4302 into python:main Sep 30, 2024
3 checks passed
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.

Tests: Silence naive datetime warning spam
3 participants