Converted datetime strings to datetime objects #10
Conversation
|
Warning Rate Limit Exceeded@Seluj78 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 10 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. WalkthroughThe recent update enhances the Changes
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- shellhub/models/device.py (3 hunks)
Additional comments: 3
shellhub/models/device.py (3)
- 44-44: Ensure all external calls to
ShellHubDevice'slast_seenattribute are updated to handledatetimeobjects instead of strings.- 48-48: Ensure all external calls to
ShellHubDevice'sstatus_updated_atattribute are updated to handledatetimeobjects instead of strings.- 49-49: Ensure all external calls to
ShellHubDevice'screated_atattribute are updated to handledatetimeobjects instead of strings.
Seluj78
left a comment
Seluj78
left a comment
There was a problem hiding this comment.
One comment
Along with that you are missing an update on the version found in __init__.py and to update unit tests :)
|
Hello @swapnil-veer! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-02-09 09:56:59 UTC |
|
Are that's all with this issue? |
|
No ! As you can see, the tests aren't passing so you'll need to fix that, as well as add some tests. There is also the review from coderabbit you need to implement |
|
@swapnil-veer Don't hesitate to let me know if you are lost and tell me where you are lost I'll be happy to guide you. Otherwise I can always edit the PR myself. |
|
@Seluj78 |
|
@swapnil-veer Before fixing the tests, look at the comments @coderabbitai made on your code and implement them (Checking if the value passed is a correct ISO datetime string.) Once you have done that, you can easily fix the tests (They are failing because they aren't an ISO format, so maybe |
|
@Seluj78 To ensure that the values passed are correct ISO datetime strings, you can use from datetime import datetime
def convert_iso_datetime(iso_string):
try:
return datetime.fromisoformat(iso_string)
except ValueError:
# Handle the error, e.g., by logging or raising a custom exception
print(f"Invalid ISO datetime string: {iso_string}")
return NoneThis method will attempt to convert the string to a |
|
@coderabbitai |
|
@swapnil-veer You're correct that |
|
I think first of all, you need to make sure that the data passed from shellhub is indeed a ISO format, otherwise this won't work at all |
|
@swapnil-veer You're correct that |
|
Hello @Seluj78 |
Seluj78
force-pushed
the
datetime_str_to_datetime_object
branch
2 times, most recently
from
26e994f to
3e8814f
Compare
|
@swapnil-veer Done :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- shellhub/init.py (1 hunks)
- shellhub/models/device.py (3 hunks)
- tests/test_devices.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- shellhub/models/device.py
Additional comments: 6
shellhub/__init__.py (1)
- 2-2: Version update to "0.3.0" correctly reflects the significant changes made, adhering to Semantic Versioning.
tests/test_devices.py (5)
- 2-2: Import of
datetimemodule is correctly added to support the conversion of datetime strings todatetimeobjects in tests.- 121-121: Conversion of
last_seento adatetimeobject usingdatetime.fromisoformat()is correctly implemented.- 125-125: Conversion of
status_updated_atto adatetimeobject usingdatetime.fromisoformat()is correctly implemented.- 126-126: Conversion of
created_atto adatetimeobject usingdatetime.fromisoformat()is correctly implemented.- 131-162: The addition of
test_get_incorrect_datetime_formatcorrectly tests the SDK's behavior when encountering incorrect datetime formats, ensuring robust error handling.
|
Now as you can see the tests fail on Python 3.7, 3.8, 3.9 and 3.10 which means something has changed on the datetime parsing since 3.11 and 3.12 are working. see https://www.reddit.com/r/Python/comments/163ugsm/psa_as_of_python_311_datetimefromisoformat/ |
|
@swapnil-veer would you like to try and add a fix for this ? |
|
I think not getting close to solution |
|
@Seluj78 |
I've already implemented that in the code, I've pushed on the branch
This already exists thanks to tox. I recommend you look up on how to use it
Because this is an SDK that connects to an external API made by the @shellhub-io team (https://shellhub.io) so we don't need to connect to their database, just interact with their API |
|
@swapnil-veer I will take over this PR and fix the last bugs, you'll be able to see in the commits how I did it :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- shellhub/init.py (1 hunks)
- shellhub/models/device.py (3 hunks)
- tests/test_devices.py (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- shellhub/init.py
- shellhub/models/device.py
Additional comments: 2
tests/test_devices.py (2)
- 122-169: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [110-130]
The conversion to
datetimeobjects forlast_seen,status_updated_at, andcreated_atattributes is correctly implemented. This aligns with the PR's objective to enhance date and time handling.
- 135-166: The implementation of
test_get_incorrect_datetime_formatcorrectly tests the handling of incorrect datetime formats by expecting aShellHubApiError. This addition enhances the robustness of the SDK by ensuring proper error handling.
Seluj78
force-pushed
the
datetime_str_to_datetime_object
branch
from
837940e to
93cb35a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
+ Coverage 81.27% 82.18% +0.90%
==========================================
Files 4 4
Lines 235 247 +12
Branches 28 30 +2
==========================================
+ Hits 191 203 +12
Misses 36 36
Partials 8 8 ☔ View full report in Codecov by Sentry. |
Seluj78
force-pushed
the
datetime_str_to_datetime_object
branch
from
93cb35a to
12abdeb
Compare
Fix issue at status_updated_at Fix: PEP 8 correction Fix: Version correction Added check for datetime string format + tests
Seluj78
force-pushed
the
datetime_str_to_datetime_object
branch
from
12abdeb to
06a3355
Compare
Seluj78
force-pushed
the
datetime_str_to_datetime_object
branch
from
06a3355 to
530f211
Compare
4 participants
Here I have used datetime module. By which we can take inputs in date format.
Fixes #3
Summary by CodeRabbit
datetimemodule in test files.datetimeobjects in test files for improved testing accuracy.