-
Notifications
You must be signed in to change notification settings - Fork 924
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
CL parser: add snapshot test and remove usage of arrow module #6517
CL parser: add snapshot test and remove usage of arrow module #6517
Conversation
This reverts commit 20ba103.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to minify this .json mock file: after minification the file size went from 58K to 37K. But I decided to revert that commit and keep the file unminified, primarily because of readability. The commit with the minified version is 20ba103. Happy to revert back to the minified version, if it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think either is fine, the point of the snapshot tests is not really to see if they match the expected input data (I sure hope they do though) but rather to ensure no accidental regressions are made in future edits.
So readability is not really a concern IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working on the parser code, I have found it helpful to be able to see what the response from the external api actually looks like. So, at least for me, the .json mock file also served this purpose, as it contains a sample response from the external api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Looks like it works as expected!
I'm not a huge fan of strptime and strftime so I left suggestions that use the iso builtin versions instead.
Co-authored-by: Viktor Andersson <30777521+VIKTORVAV99@users.noreply.github.com>
Co-authored-by: Viktor Andersson <30777521+VIKTORVAV99@users.noreply.github.com>
Just applied your suggestions related to the use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for doing this and special thanks for adding tests!
Issue
#6135
Description
This PR adds a snapshot test for the CL parser and removes usage of the
arrow
module from the CL parser.Preview
Double check
poetry run test_parser "zone_key"
pnpx prettier@2 --write .
andpoetry run format
in the top level directory to format my changes.