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: remove arrow from CO parser #6573

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nboswell216
Copy link
Contributor

Issue #6135

Description

Removes arrow dependency from CO parser.
Note: It seems the 'fecha' datapoint sometimes is missing the full millisecond precision, so there is a check to add a trailing zero if that is the case

Preview

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@github-actions github-actions bot added parser python Pull requests that update Python code labels Mar 23, 2024
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Several small fixes needed but looks to be on the right track!

Thanks for upgrading this parser.

parsers/CO.py Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
@nboswell216
Copy link
Contributor Author

@VIKTORVAV99 I researched the difference between .replace() and astimezone() for changing timezones and indeed it looks like astimezone() is the preferred method. I used replace() in my refactor for both CA_AB and CA_BC - do you want me to go back and update those to astimezone(), or are you ok with those as they are?

@VIKTORVAV99
Copy link
Member

@VIKTORVAV99 I researched the difference between .replace() and astimezone() for changing timezones and indeed it looks like astimezone() is the preferred method. I used replace() in my refactor for both CA_AB and CA_BC - do you want me to go back and update those to astimezone(), or are you ok with those as they are?

In general I think where we used .replace() in arrow we should still use replace and where arrow used .to() we should use .astimezone() instead.

The absolut best way to ensure there is no missmatch between the two would be to add snapshot tests before modifying them and then making sure the date times don't change (more than the tz info).

Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Two small comments, otherwise it looks great!

parsers/CO.py Outdated Show resolved Hide resolved
parsers/CO.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants