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

configurable date format and handle decimal InvalidOperation #20

Closed
wants to merge 5 commits into from

Conversation

aroder
Copy link

@aroder aroder commented Feb 10, 2022

  • Adds a property date_format that makes the target key's date format configurable. It defaults to the original hard-coded date format, making this change backwards compatible.
  • Adds a line to adjust the decimal precision if it is too small compared to the tap's schema

@ome9ax are you open to a PR for this, or open to discuss?

@aroder
Copy link
Author

aroder commented Mar 10, 2022

Hi @ome9ax bumping this. Do you have time to review?

Copy link
Owner

@ome9ax ome9ax left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @aroder
Left a note, Let me know your thoughts.

Comment on lines +4 to +5
pytest-cov
moto[s3]
Copy link
Owner

Choose a reason for hiding this comment

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

Those are already in the [options.extras_require] section L34-35. I keep them apart as they are not needed to run the package, only to run the tests.
My rational is to keep the target lean if possible.

So I don't know how much of a best practice it is but I used

pip install --upgrade .[test,lint,dist]

To install what's needed on local. It was my way of following the KISS principle here.

@@ -13,12 +13,13 @@

from jsonschema import Draft4Validator, FormatChecker
from decimal import Decimal
from adjust_precision_for_schema import adjust_decimal_precision_for_schema
Copy link
Owner

Choose a reason for hiding this comment

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

That a great one. Thanks for adding it.

Copy link
Owner

Choose a reason for hiding this comment

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

Reading the Handle multipleOf overflow fix introduced in jsonschema v4.0.0, I wonder if that update doesn't make adjust_precision_for_schema reduntant?

@haleemur
Copy link
Contributor

I would like to note that the current implementation allows for specifying the date format already. the additional parameter might be redundant.

e.g.

naming_convention: "{stream}/export_date={date:%Y-%m-%d}/{timestamp}-{stream}.json"

however, we should add some parsing logic to ensure that the naming convention provided is valid and update the documentation with some examples.

@ome9ax
Copy link
Owner

ome9ax commented Sep 2, 2022

adjust-precision-for-schema merged in #75 and available in the v1.2.2 release @aroder .
Also closes #69 .
I believe this PR can now be closed unless reasons not to.

@ome9ax
Copy link
Owner

ome9ax commented Oct 3, 2022

All the functionalities are now implemented.

@ome9ax ome9ax closed this Oct 3, 2022
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.

3 participants