Use timezone information from the api package event.#12
Use timezone information from the api package event.#12dimarobert wants to merge 2 commits intoshaiu:devfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @dimarobert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the accuracy of timestamp handling by integrating timezone information directly from the API response. It also refines the internal Package model to natively support datetime objects, streamlining data processing and ensuring timestamps are correctly localized and converted to UTC.
Highlights
- Timezone Information Extraction: The pull request now reads and processes timezone information from the FLastEvent.dd property within the /orderapi/call API's GetTrackInfoList method. This ensures more accurate timestamp handling.
- datetime Object Support: The Package.timestamp attribute now supports direct assignment of datetime objects, improving flexibility and type handling within the pyseventeentrack library.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request enhances timezone handling by processing timezone information from the API and adds support for datetime objects as timestamps. The changes look good, but I have a couple of suggestions. In pyseventeentrack/profile.py, the exception handling for parsing the new timestamp format could be more specific to avoid hiding potential bugs. In pyseventeentrack/package.py, the timestamp string parsing logic could be refactored to be more readable and maintainable. As you mentioned in the description, adding tests for the new timezone parsing logic would be great to ensure its correctness and prevent future regressions. Modifying the existing fixture or adding a new one as you suggested would be a good approach. Please see my detailed comments below.
| if dd: | ||
| try: | ||
| dt_str = f"{dd['d']}T{dd['t']}{dd['tz']}" | ||
| timestamp = datetime.fromisoformat(dt_str) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The if dd: check is not robust enough, as dd could be a non-dictionary truthy value. Using isinstance(dd, dict) is safer. Additionally, catching a broad Exception is discouraged as it can hide unexpected errors. It's better to catch specific exceptions like KeyError, ValueError, and TypeError that you might expect from this block of code.
| if dd: | |
| try: | |
| dt_str = f"{dd['d']}T{dd['t']}{dd['tz']}" | |
| timestamp = datetime.fromisoformat(dt_str) | |
| except Exception: | |
| pass | |
| if isinstance(dd, dict): | |
| try: | |
| dt_str = f"{dd['d']}T{dd['t']}{dd['tz']}" | |
| timestamp = datetime.fromisoformat(dt_str) | |
| except (KeyError, ValueError, TypeError): | |
| pass |
| try: | ||
| timestamp = tz.localize( | ||
| datetime.strptime(self.timestamp, "%Y-%m-%d %H:%M:%S") | ||
| datetime.strptime(self.timestamp, "%Y-%m-%d %H:%M") | ||
| ) | ||
| except ValueError: | ||
| timestamp = datetime(1970, 1, 1, tzinfo=UTC) | ||
| try: | ||
| timestamp = tz.localize( | ||
| datetime.strptime(self.timestamp, "%Y-%m-%d %H:%M:%S") | ||
| ) | ||
| except ValueError: | ||
| timestamp = datetime(1970, 1, 1, tzinfo=UTC) |
There was a problem hiding this comment.
This nested try-except block for parsing different timestamp formats can be simplified and made more extensible by iterating over a list of possible formats. This approach flattens the code, making it easier to read and to add more formats in the future.
| try: | |
| timestamp = tz.localize( | |
| datetime.strptime(self.timestamp, "%Y-%m-%d %H:%M:%S") | |
| datetime.strptime(self.timestamp, "%Y-%m-%d %H:%M") | |
| ) | |
| except ValueError: | |
| timestamp = datetime(1970, 1, 1, tzinfo=UTC) | |
| try: | |
| timestamp = tz.localize( | |
| datetime.strptime(self.timestamp, "%Y-%m-%d %H:%M:%S") | |
| ) | |
| except ValueError: | |
| timestamp = datetime(1970, 1, 1, tzinfo=UTC) | |
| timestamp_formats = ["%Y-%m-%d %H:%M", "%Y-%m-%d %H:%M:%S"] | |
| parsed_dt = None | |
| for fmt in timestamp_formats: | |
| try: | |
| parsed_dt = datetime.strptime(self.timestamp, fmt) | |
| break | |
| except ValueError: | |
| pass | |
| if parsed_dt: | |
| timestamp = tz.localize(parsed_dt) | |
| else: | |
| timestamp = datetime(1970, 1, 1, tzinfo=UTC) |
|
@shaiu For me it looks like the AI Code Review was a miss. In the first recommendation the idea behind the catch-all exception is to fallback to the old behavior so I would say it should stay. Maybe the Let me know if you need anything more on the PR. |
| dd = event.get("dd") | ||
| if dd: | ||
| try: | ||
| dt_str = f"{dd['d']}T{dd['t']}{dd['tz']}" |
There was a problem hiding this comment.
| dt_str = f"{dd['d']}T{dd['t']}{dd['tz']}" | |
| dt_str = f"{dd['d']}T{dd['t']}{dd.get('tz') or ''}" |
I noticed in some scenarios that the tz can be null in the json object, so maybe this is better to avoid a datetime parsing exception? (the result should be the same either way as timestamp would preserve its initial event.get("a") value which in this case would be identical to the one from the formatted string)
Describe what the PR does:
Reads and processes the timezone information from the /orderapi/call {method: GetTrackInfoList} API present in the
Json.[].FLastEvent.ddproperty.Also adds support for receiving
datetimeobjects as value forPackage.timestamp.I did not add any new tests, let me know if simply duplicating the first four package entries from tests/fixtures/packages_response.json and adding the additional
ddinformation would be enough of a test suite for the changes, or you want a completely separate test case (and fixture file).Does this fix a specific issue?
Fixes #11
Checklist:
README.mdwith any new documentation.AUTHORS.md.