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

CDK: Add support for input format parsing at jinja macro format_datetime #40759

Merged
merged 8 commits into from
Jul 8, 2024

Conversation

cmm-airbyte
Copy link
Contributor

What

In order to support complex input datetime formats at low code, including input_format param to format_datetime macro, so it will allow us to parse any kind of datetime string, so it can be processed to standarized ISO (or any other format as required).

Review guide

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Jul 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jul 8, 2024 8:32am

@cmm-airbyte cmm-airbyte requested a review from girarda July 5, 2024 21:51
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


cristina.mariscal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Jul 5, 2024
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

minor nit. looks good :shipit:

@@ -4,7 +4,7 @@ build-backend = "poetry.core.masonry.api"

[tool.poetry]
name = "airbyte-cdk"
version = "2.2.0"
version = "2.2.1"
Copy link
Contributor

@girarda girarda Jul 5, 2024

Choose a reason for hiding this comment

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

you shouldn't need to bump the version. the github action will take care of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -107,7 +107,7 @@ def duration(datestring: str) -> Union[datetime.timedelta, isodate.Duration]:
return parse_duration(datestring) # type: ignore # mypy thinks this returns Any for some reason


def format_datetime(dt: Union[str, datetime.datetime], format: str) -> str:
def format_datetime(dt: Union[str, datetime.datetime], format: str, input_format: str = None) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: input_format is an Optional[str]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cristina.mariscal added 2 commits July 5, 2024 16:14
@cmm-airbyte cmm-airbyte enabled auto-merge (squash) July 5, 2024 22:28
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the comments!

Copy link
Contributor

@aldogonzalez8 aldogonzalez8 left a comment

Choose a reason for hiding this comment

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

APPROVED

@@ -107,7 +107,7 @@ def duration(datestring: str) -> Union[datetime.timedelta, isodate.Duration]:
return parse_duration(datestring) # type: ignore # mypy thinks this returns Any for some reason


def format_datetime(dt: Union[str, datetime.datetime], format: str) -> str:
def format_datetime(dt: Union[str, datetime.datetime], format: str, input_format: Optional[str] = None) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are errors for this test:

==================================== ERRORS ====================================
_ ERROR collecting airbyte-cdk/python/unit_tests/sources/declarative/interpolation/test_macros.py _
airbyte-cdk/python/unit_tests/sources/declarative/interpolation/test_macros.py::test_format_datetime: in "parametrize" the number of names (4):
  ['test_name', 'input_value', 'format', 'expected_output']
must be equal to the number of values (5):
  ('test_datetime_string_to_date', '2022-01-01T01:01:01Z', '%Y-%m-%d', None, '2022-01-01')
=============================== warnings summary ===============================
airbyte_cdk/connector_builder/connector_builder_handler.py:28

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cmm-airbyte cmm-airbyte merged commit c4b8212 into master Jul 8, 2024
26 of 27 checks passed
@cmm-airbyte cmm-airbyte deleted the cmm-airbyte/macros_dt_str branch July 8, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants