Skip to content

Conversation

@Dev232001
Copy link
Contributor

@Dev232001 Dev232001 commented Feb 24, 2022

closes: #21683
Replace all the days_ago functions with datetime functions as asked in the issue

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:core-operators area:Scheduler including HA (high availability) scheduler labels Feb 24, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 24, 2022

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

Comment on lines 18 to +19
from datetime import timedelta

import datetime
Copy link
Contributor

Choose a reason for hiding this comment

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

If from datetime import timedelta already present why not doing
from datetime import datetime, timedelta

and use datetime(2022,1,1) rather than datetime.datetime(2022,1,1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! didn't see that there. There are a lot of files and I checked each one manually for maximum efficiency so forgot to use some of the already present libraries. If you want I can change them again quickly

dag_id="TEST_DAG_ID",
default_args=dict(
start_date=days_ago(2),
start_date=datetime.datetime(2022, 1, 1),
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we should move away from datetime.datetime to pendulum.datetime for start_date values in unit tests too based on the conversation in #21646. @eladkal WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean just for tests or also for example dags?
I have no objection.

Copy link
Contributor

Choose a reason for hiding this comment

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

For unit tests as well as example DAGs for consistency in tests, examples, docs, etc. The core example DAGs have been transitioned but the provider ones haven't yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I too think pendulum is a better alternative for datetime

Copy link
Contributor

Choose a reason for hiding this comment

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

Then yeah lets do that

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course! Not a problem at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80a6ae5

Another pull request after swapping datetime with pendulum where start dates are involved

Copy link
Member

Choose a reason for hiding this comment

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

For tests specifically I actually prefer datetime.datetime since it better ensures we are coercing timezones correctly internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one is also done in the previous pull request. I guess my task is done so please let me know if there is anything more to update.

kaxil added a commit to astronomer/astronomer-providers that referenced this pull request Mar 1, 2022
@uranusjr
Copy link
Member

uranusjr commented Mar 1, 2022

Please fix the static check failures.

kaxil added a commit to astronomer/astronomer-providers that referenced this pull request Mar 1, 2022
@Dev232001
Copy link
Contributor Author

Dev232001 commented Mar 1, 2022 via email

@Dev232001
Copy link
Contributor Author

I tried solving the static checks but wasn't able to do so. Can you help me on what is wrong exactly

@Bowrna
Copy link
Contributor

Bowrna commented Mar 6, 2022

@Dev232001 run pre-commit run --all-files locally in your machine to fix the black formatting, sorting issues. There are a few duplicate imports too. If you run the command locally it will point to the files with those errors. you can fix it and commit to avoid static check issues.

Alternatively you can run static check via Breeze too. Under the hood this runs pre-commit too.
https://github.com/apache/airflow/blob/main/BREEZE.rst#running-static-checks

@eladkal
Copy link
Contributor

eladkal commented Mar 18, 2022

@Dev232001 do you need further help?

@github-actions
Copy link

github-actions bot commented May 3, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 3, 2022
@uranusjr
Copy link
Member

uranusjr commented May 3, 2022

Superceded by #23237.

@uranusjr uranusjr closed this May 3, 2022
OlympuJupiter added a commit to OlympuJupiter/astronomer-providers that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:CLI area:Scheduler including HA (high availability) scheduler stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove all usages of days_ago from test suite

5 participants