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

Add epoch as an option for file_template #1028

Closed
wants to merge 3 commits into from
Closed

Add epoch as an option for file_template #1028

wants to merge 3 commits into from

Conversation

ccrvlh
Copy link
Contributor

@ccrvlh ccrvlh commented May 2, 2022

Hi all, as we are not huge fans of big file names, we would rather use epoch as a way to have the migrations in chronological order. I made a small change (with docs and test) to allow the epoch to be used in the file_template. From what I could see, this shouldn't have many side effects. As this is my first PR around here, please let me know if there's anything that I should do differently.

Description

  • Added epoch as an option to the file_template.
  • Added a test to make sure the epoch is considered on the name
  • Added a test to make sure the epoch formula is correct (one extra second increases epoch by 1)

Checklist

This pull request is:

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

we can add this but let's use std library

@@ -782,6 +782,8 @@ def _rev_path(
message: Optional[str],
create_date: "datetime.datetime",
) -> str:
epoch_reference_date = datetime.datetime(1970, 1, 1)
epoch = int((create_date - epoch_reference_date).total_seconds())
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not hardcode this here. you can call create_date.timestamp():

>>> import time
>>> import datetime
>>> create_date = datetime.datetime.now()
>>> epoch_reference_date = datetime.datetime(1970, 1, 1)
>>> epoch = int((create_date - epoch_reference_date).total_seconds())
>>> epoch
1651522726
>>> create_date.timestamp()
1651537126.489675

Copy link
Member

Choose a reason for hiding this comment

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

I think the latter is using UTC which accounts for the discrepancy

Copy link
Contributor Author

@ccrvlh ccrvlh May 3, 2022

Choose a reason for hiding this comment

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

Sure, the reason I chose that path was because going with timestamp() would need a round(..., 0) and the conversion to int to avoid the decimals, but I guess it should work without problems.

Do you think this int(round(create_date.timestamp(), 0)) could work?

>>> import time
>>> import datetime
>>> create_date = datetime.datetime.now()
>>> epoch_reference_date = datetime.datetime(1970, 1, 1)
>>> epoch = int((create_date - epoch_reference_date).total_seconds())
>>> epoch
1651527473
>>> create_date.timestamp()
1651538273.443539
>>> int(round(create_date.timestamp(),0))
1651538273
>>> int(create_date.timestamp())
1651538273

Actually I think just the int conversion would be enough int(create_date.timestamp())

Copy link
Member

Choose a reason for hiding this comment

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

yeah just int it, i think that's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great, just pushed the new version and fixes tests, everything seems to pass.
thanks a lot for the fast review @zzzeek

Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

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

looks ok, but the test fail, could you take a look?

You find the failure in the action checks

@ccrvlh
Copy link
Contributor Author

ccrvlh commented May 3, 2022

Sure, just found out it is because of the timezone, I'm GMT -3 and tests pass locally, so when running on GMT the epochs differ, I'll take a look on how to fix this.

@zzzeek
Copy link
Member

zzzeek commented May 3, 2022

Sure, just found out it is because of the timezone, I'm GMT -3 and tests pass locally, so when running on GMT the epochs differ, I'll take a look on how to fix this.

try to use all GMT for the source datetime objects that are created in the test.

@ccrvlh
Copy link
Contributor Author

ccrvlh commented May 3, 2022

I think that adding the tzinfo (and adding the correct utc epoch) works: create_date = datetime.datetime(2012, 7, 25, 15, 8, 5, tzinfo=tz.gettz("UTC")) I'll test in a vagrant box in UTC to see make sure. Passes locally now with the right epoch.

@CaselIT CaselIT requested a review from sqla-tester May 3, 2022 17:20
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 27970cd of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 27970cd: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3823

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

thanks for the contribution!

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3823

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3823 has been merged. Congratulations! :)

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.

Add epoch as an option for the file_template
4 participants