Skip to content

Conversation

@tkdrob
Copy link
Owner

@tkdrob tkdrob commented Sep 7, 2024

Proposed change

This PR attempts to fix the error in getting the wrong date for calendar items. All dates are now parsed as UTC if a timezone is not provided in the input string.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass.
  • There is no commented out code in this PR.
  • The code has been formatted (make lint)
  • Tests have been added to verify that the new code works.

Summary by CodeRabbit

  • New Features

    • Enhanced datetime handling across the application, ensuring consistency and accuracy in date and time processing.
    • Introduced a new categorization for release types, improving clarity in release management.
  • Bug Fixes

    • Resolved potential issues with datetime comparisons by ensuring all datetime objects are timezone-aware.
  • Tests

    • Updated test cases to consistently use UTC timezone for datetime assertions, improving test reliability and accuracy.

@coderabbitai
Copy link

coderabbitai bot commented Sep 7, 2024

Warning

Rate limit exceeded

@tkdrob has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 56 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between b6b82b8 and cb7e6cc.

Walkthrough

The changes involve modifications to datetime handling across multiple files in the aiopyarr project. Key updates include the simplification of the get_datetime function, the removal of the get_date function, and the introduction of new constants for release types. Additionally, various test files have been updated to ensure consistent timezone handling using UTC, enhancing the accuracy of datetime assertions throughout the codebase.

Changes

Files Change Summary
aiopyarr/models/base.py Simplified get_datetime function by removing utc parameter; removed get_date function.
aiopyarr/models/const.py Replaced CONVERT_TO_DATE with RELEASE_TYPES; modified CONVERT_TO_DATETIME to include RELEASE_TYPES.
aiopyarr/models/radarr_common.py Updated releaseDateType method to accept datetime instead of date, enhancing date handling.
tests/test_lidarr.py Updated datetime assertions to include UTC timezone, ensuring consistency across tests.
tests/test_radarr.py Replaced date with datetime in assertions to ensure timezone awareness.
tests/test_request.py Modified datetime assertions to include UTC timezone for accuracy.
tests/test_sonarr.py Transitioned to using UTC for timezone handling in assertions, enhancing compatibility.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Application
    participant Database

    User->>Application: Request datetime handling
    Application->>Database: Fetch datetime data
    Database-->>Application: Return datetime data
    Application->>User: Provide datetime response
Loading

🐰 In the meadow where I hop and play,
The datetime changes brightened my day!
With UTC now guiding the way,
No more confusion, come what may!
Hooray for constants, clear and bright,
In the world of code, all feels just right! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 683c4a2 and b6b82b8.

Files selected for processing (7)
  • aiopyarr/models/base.py (4 hunks)
  • aiopyarr/models/const.py (1 hunks)
  • aiopyarr/models/radarr_common.py (2 hunks)
  • tests/test_lidarr.py (35 hunks)
  • tests/test_radarr.py (21 hunks)
  • tests/test_request.py (13 hunks)
  • tests/test_sonarr.py (28 hunks)
Additional context used
Ruff
aiopyarr/models/radarr_common.py

152-152: Do not perform function call datetime.now in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

Additional comments not posted (44)
aiopyarr/models/const.py (2)

28-30: New constant RELEASE_TYPES added.

The addition of RELEASE_TYPES is appropriate for categorizing different types of releases. Ensure that this new constant is used consistently across the application.

Run the following script to verify the usage of RELEASE_TYPES:

Verification successful

Usage of RELEASE_TYPES is consistent and appropriate.

The RELEASE_TYPES constant is used in aiopyarr/models/radarr_common.py to iterate over its values, confirming its role in handling different release types. This usage aligns with its intended purpose. No issues found.

  • aiopyarr/models/radarr_common.py: Iterates over RELEASE_TYPES to handle release types.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all usages of `RELEASE_TYPES` across the application.

# Test: Search for the constant usage. Expect: Consistent usage across the application.
rg --type python -A 5 $'RELEASE_TYPES'

Length of output: 68


Script:

#!/bin/bash
# Description: Verify all usages of `RELEASE_TYPES` across the application.

# Test: Search for the constant usage. Expect: Consistent usage across the application.
rg --type py -A 5 $'RELEASE_TYPES'

Length of output: 1298


35-35: Expansion of CONVERT_TO_DATETIME to include RELEASE_TYPES.

The inclusion of RELEASE_TYPES in CONVERT_TO_DATETIME is a logical extension to handle various release dates. Verify that this change does not impact existing datetime parsing and handling negatively.

Run the following script to verify the impact of this change on datetime handling:

aiopyarr/models/base.py (2)

23-28: Removal of get_date function.

The removal of get_date could potentially impact parts of the codebase that relied on this function. Verify that this removal does not break any existing functionality.

Run the following script to verify the impact of removing get_date:

Verification successful

Removal of get_date function verified.

The get_date function is not used anywhere in the codebase, indicating that its removal does not impact any existing functionality. No further action is required.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing `get_date`.

# Test: Search for the usage of `get_date`. Expect: No broken functionality due to its removal.
rg --type python -A 5 $'get_date'

Length of output: 63


Script:

#!/bin/bash
# Description: Verify the impact of removing `get_date`.

# Test: Search for the usage of `get_date`. Expect: No broken functionality due to its removal.
rg --type py -A 5 $'get_date'

Length of output: 1621


23-28: Simplification of get_datetime function.

The removal of the utc parameter simplifies the function but requires verification to ensure it does not negatively impact datetime handling, especially in contexts where UTC conversion was previously necessary.

Run the following script to verify the impact of this change on datetime handling:

aiopyarr/models/radarr_common.py (1)

151-153: Enhancement of releaseDateType method.

The change to accept a datetime object and default to the current time in UTC enhances the method's precision. Verify that this change does not negatively impact date handling or introduce regressions.

Run the following script to verify the impact of this change on date handling:

Verification successful

Verification of releaseDateType method change.

The change to accept a datetime object and default to the current time in UTC is verified by existing test cases in tests/test_radarr.py. These tests cover various datetime scenarios, ensuring that the method handles date inputs correctly without introducing regressions. No further action is required.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the changes to `releaseDateType`.

# Test: Search for the usage of `releaseDateType`. Expect: No negative impact on date handling.
rg --type python -A 5 $'releaseDateType'

Length of output: 70


Script:

#!/bin/bash
# Corrected script to verify the impact of the changes to `releaseDateType`.

# Search for the definition of `releaseDateType` in the codebase.
rg --type py -A 5 'def releaseDateType'

# Search for the usage of `releaseDateType` across the codebase.
rg --type py -A 5 'releaseDateType('

Length of output: 625


Script:

#!/bin/bash
# Corrected script to verify the impact of the changes to `releaseDateType`.

# Search for the definition of `releaseDateType` in the codebase.
rg --type py -A 5 'def releaseDateType'

# Search for the usage of `releaseDateType` across the codebase with escaped parentheses.
rg --type py -A 5 'releaseDateType\('

Length of output: 2529

Tools
Ruff

152-152: Do not perform function call datetime.now in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

tests/test_sonarr.py (14)

6-11: Refactor: Improved timezone handling using UTC.

The changes to import UTC directly from the datetime module, with a fallback to timezone.utc if UTC is not available, are a good practice for clarity and compatibility. This approach ensures that the code is more robust across different Python versions where UTC might not be directly available.


68-68: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for airDateUtc aligns with the new import strategy and ensures consistent timezone handling across the tests.


124-124: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for airDateUtc aligns with the new import strategy and ensures consistent timezone handling across the tests.


140-142: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for dateAdded aligns with the new import strategy and ensures consistent timezone handling across the tests.


248-248: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for airDateUtc aligns with the new import strategy and ensures consistent timezone handling across the tests.


255-257: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for dateAdded aligns with the new import strategy and ensures consistent timezone handling across the tests.


310-310: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for firstAired aligns with the new import strategy and ensures consistent timezone handling across the tests.


318-318: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for added aligns with the new import strategy and ensures consistent timezone handling across the tests.


650-652: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for estimatedCompletionTime aligns with the new import strategy and ensures consistent timezone handling across the tests.


1279-1279: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for airDateUtc aligns with the new import strategy and ensures consistent timezone handling across the tests.


1299-1301: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for estimatedCompletionTime aligns with the new import strategy and ensures consistent timezone handling across the tests.


1383-1383: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for firstAired aligns with the new import strategy and ensures consistent timezone handling across the tests.


1391-1391: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for added aligns with the new import strategy and ensures consistent timezone handling across the tests.


1402-1402: Update: Consistent timezone handling in datetime assertions.

The update to use UTC for timezone information in the datetime assertion for airDateUtc aligns with the new import strategy and ensures consistent timezone handling across the tests.

tests/test_radarr.py (18)

6-11: Ensure proper handling of the UTC import.

The conditional import of UTC from datetime is a good approach to ensure compatibility across different Python environments. This change is crucial for maintaining timezone awareness throughout the tests.


65-65: Correct implementation of timezone-aware datetime.

The assertion for data.records[0].date correctly uses datetime with tzinfo=UTC, aligning with the PR's goal to ensure timezone awareness. This is a crucial change for consistency in datetime handling across different time zones.


145-145: Proper use of timezone-aware datetime in assertions.

The assertion for data[0].date correctly uses datetime with tzinfo=UTC. This change ensures that the tests remain consistent and accurate when dealing with time zones, which is essential for the application's functionality across different regions.


179-182: Ensure consistent timezone handling in calendar-related assertions.

The use of datetime with tzinfo=UTC for physicalRelease, digitalRelease, and releaseDateType ensures that the tests are robust and timezone-aware. This is particularly important for calendar-related data, which must be accurate and consistent across various time zones.


205-205: Correct implementation of timezone-aware datetime for added date.

The assertion for data[0].added correctly uses datetime with tzinfo=UTC. This ensures that the date added is consistently handled across different time zones, which is crucial for accurate record-keeping and functionality.


222-222: Proper handling of timezone in movie file date added.

The assertion for data[0].movieFile.dateAdded correctly uses datetime with tzinfo=UTC. This change is essential for ensuring that the date and time the movie file was added is accurately recorded and consistent across different time zones.


337-337: Accurate timezone handling for historical data retrieval.

The assertion for data.records[0].date correctly uses datetime with tzinfo=UTC. This ensures that historical data is handled accurately with respect to time zones, which is crucial for features that rely on date and time calculations.


455-459: Consistent timezone-aware datetime handling in movie release dates.

The assertions for inCinemas, physicalRelease, digitalRelease, and the method releaseDateType correctly use datetime with tzinfo=UTC. This ensures that all movie release dates are handled consistently and accurately across different time zones, aligning with the PR's objectives.


692-695: Proper timezone handling for cinema and physical release dates.

The use of datetime with tzinfo=UTC for inCinemas and physicalRelease dates, as well as the method releaseDateType, ensures accurate and consistent handling of these dates across different time zones. This is crucial for features that depend on release timing.


721-721: Correct implementation of timezone-aware datetime for added date.

The assertion for data.added correctly uses datetime with tzinfo=UTC. This ensures that the date the movie was added is consistently handled across different time zones, which is crucial for accurate record-keeping and functionality.


738-738: Proper handling of timezone in movie file date added.

The assertion for data.movieFile.dateAdded correctly uses datetime with tzinfo=UTC. This change is essential for ensuring that the date and time the movie file was added is accurately recorded and consistent across different time zones.


840-840: Accurate timezone handling for movie file date added.

The assertion for data.dateAdded correctly uses datetime with tzinfo=UTC. This ensures that the date and time the movie file was added is accurately recorded and consistent across different time zones, which is crucial for features that rely on date and time calculations.


989-991: Ensure accurate timezone handling for estimated completion time.

The assertion for data.records[0].estimatedCompletionTime correctly uses datetime with tzinfo=UTC. This is crucial for accurately predicting and displaying the estimated completion time of tasks across different time zones.


1118-1120: Consistent timezone-aware datetime handling in queue details.

The assertion for data[0].estimatedCompletionTime correctly uses datetime with tzinfo=UTC. This ensures that the estimated completion time is handled accurately and consistently across different time zones, which is essential for scheduling and time management features.


1218-1224: Accurate handling of movie release dates with timezone awareness.

The use of datetime with tzinfo=UTC for inCinemas, physicalRelease, digitalRelease, and the method releaseDateType ensures that all movie release dates are handled consistently and accurately across different time zones. This aligns with the PR's objectives to improve date handling.


1246-1246: Proper implementation of timezone-aware datetime for added date.

The assertion for data.movie.added correctly uses datetime with tzinfo=UTC. This ensures that the date the movie was added is consistently handled across different time zones, which is crucial for accurate record-keeping and functionality.


1263-1265: Accurate timezone handling for movie file date added.

The assertion for data.movie.movieFile.dateAdded correctly uses datetime with tzinfo=UTC. This change is essential for ensuring that the date and time the movie file was added is accurately recorded and consistent across different time zones.


1357-1357: Ensure accurate timezone handling for release publish date.

The assertion for data[0].publishDate correctly uses datetime with tzinfo=UTC. This is crucial for accurately displaying the publish date of releases across different time zones, aligning with the PR's objectives to improve date handling.

tests/test_lidarr.py (5)

7-12: Review: Import and Fallback for UTC Timezone

The import block for the UTC timezone with a fallback to timezone.utc is a robust solution for ensuring that the tests use timezone-aware datetime objects. This change is crucial for the consistency of datetime comparisons across different environments and is well implemented here.


84-84: Review: Timezone Awareness in Datetime Assertion

The addition of tzinfo=UTC to the datetime object in the assertion at line 84 is a necessary improvement to ensure that the tests are reliable across different timezone settings. This change aligns with the PR's goal to handle datetime values consistently and accurately.


127-127: Review: Consistent Timezone in Datetime Assertion

The update to include tzinfo=UTC in the datetime assertion at line 127 ensures that the datetime objects are timezone aware, which is crucial for accurate test results across various environments. This change is in line with the PR's objectives to enhance datetime handling.


299-299: Review: Comprehensive Timezone Handling in Datetime Assertions

The inclusion of tzinfo=UTC in the datetime assertions across various lines (299, 315-317, 330, 346-348, 367) ensures that all datetime comparisons are timezone aware. This comprehensive update is crucial for maintaining the accuracy and reliability of the tests across different environments.

Also applies to: 315-317, 330-330, 346-348, 367-367


515-515: Review: Uniform Timezone Specification Across Tests

The consistent application of tzinfo=UTC across all datetime assertions in various test functions throughout the file ensures uniform timezone handling. This change is essential for the accuracy of datetime comparisons in tests, particularly in a distributed environment where servers might be located in different time zones.

Also applies to: 558-558, 606-606, 681-681, 724-724, 870-870, 913-913, 1201-1201, 1210-1212, 1236-1236, 1317-1319, 1513-1515, 1614-1614, 1639-1639, 1708-1710, 1928-1928, 1953-1953, 1996-1998, 2146-2146, 2276-2276, 2314-2314, 2345-2345

tests/test_request.py (2)

7-12: Review of conditional import for UTC

The conditional import of UTC from the datetime module is a robust approach to ensure compatibility across different Python environments. This is particularly useful in environments where UTC is not available directly under datetime but can be accessed through timezone.utc. This change enhances the robustness of the code by providing a fallback mechanism.


103-103: Review of datetime assertions with timezone

The modifications to the datetime assertions in various test cases ensure that all datetime comparisons are made with explicit timezone awareness, specifically using UTC. This is crucial for maintaining consistency and accuracy in datetime handling, especially in test environments where the system timezone might differ from UTC. The changes align with the PR's objective to standardize datetime handling across the codebase.

Also applies to: 126-126, 129-129, 468-468, 488-488, 516-516, 560-560, 599-599, 642-642, 721-731

@emontnemery
Copy link

emontnemery commented Nov 7, 2024

As previously mentioned on Discord, the changes in this PR are more extensive than I expected:
Why do we now parse the dates to timezone aware objects, I think it would be more natural to just let them be dates?
From the API we get for example 'inCinemas': '2024-09-11T00:00:00Z', I'm assuming the intention is not to signal the movie was released at midnight September 11th in the UTC timezone, but that the movie is released today?

Or is the plan that the Home Assistant integration knows inCinemas, among other fields, should be interpreted as a date, and we worry about converting times to dates there?

@allenporter maybe you could also take a look since you helped with home-assistant/core#118251

@allenporter
Copy link

As previously mentioned on Discord, the changes in this PR are more extensive than I expected: Why do we now parse the dates to timezone aware objects, I think it would be more natural to just let them be dates? From the API we get for example 'inCinemas': '2024-09-11T00:00:00Z', I'm assuming the intention is not to signal the movie was released at midnight September 11th in the UTC timezone, but that the movie is released today?

Or is the plan that the Home Assistant integration knows inCinemas, among other fields, should be interpreted as a date, and we worry about converting times to dates there?

@allenporter maybe you could also take a look since you helped with home-assistant/core#118251

I agree that it seems like releaseDate would be simplest returned as a datetime.date here. You can do it in home assistant also, but doing it here seems nice. However, this might be considered a breaking change, so not sure how large of a deal this is for other clients of this API. I assume with this all home assistant will need to do is call .date() on the time though so maybe not so bad.

if key == "airDateUtc":
value = get_datetime(value, utc=True)
else:
value = get_datetime(value)

Choose a reason for hiding this comment

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

What format are the dates returned from the API in this case? and what does ciso8601.parse_datetime do with them?

I assume the logic now is something like:

  • if its a utc date, you get a utc date (then assuming .date() wil be correct)
  • if its a timesamp aware date, you get a timestamp aware date. (then assuming .date() wil be correct)
  • if its no timezone datetime, you get a utc datetime (probably reasonable, then .date() will be correct)

Choose a reason for hiding this comment

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

resolved offline.

@allenporter
Copy link

As previously mentioned on Discord, the changes in this PR are more extensive than I expected: Why do we now parse the dates to timezone aware objects, I think it would be more natural to just let them be dates? From the API we get for example 'inCinemas': '2024-09-11T00:00:00Z', I'm assuming the intention is not to signal the movie was released at midnight September 11th in the UTC timezone, but that the movie is released today?
Or is the plan that the Home Assistant integration knows inCinemas, among other fields, should be interpreted as a date, and we worry about converting times to dates there?
@allenporter maybe you could also take a look since you helped with home-assistant/core#118251

I agree that it seems like releaseDate would be simplest returned as a datetime.date here. You can do it in home assistant also, but doing it here seems nice. However, this might be considered a breaking change, so not sure how large of a deal this is for other clients of this API. I assume with this all home assistant will need to do is call .date() on the time though so maybe not so bad.

I misread the diffs. This PR is changing some of the datetime.date return values to datetime.datetime and i think it would be best to keep them as datetime.date as suggested.

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.

Radarr integration calendar entries a day out

4 participants