Skip to content

Replace strftime with f-strings where nicer - #33455

Merged
potiuk merged 1 commit into
apache:mainfrom
eumiro:no-strftime
Aug 23, 2023
Merged

potiuk merged 1 commit into
apache:mainfrom
eumiro:no-strftime

Conversation

@eumiro

@eumiro eumiro commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

No description provided.

@uranusjr

Copy link
Copy Markdown
Member

I’m not particular sure about this, strftime is more obvious and (subjectively) easier to read. The f-string format can be beneficial if the formatted date string is going into another f-string (as in some cases here) but not when it’s on its own.

@eumiro

eumiro commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

@uranusjr Reduced to being part of larger f-strings.

@potiuk

potiuk commented Aug 20, 2023 •

Copy link
Copy Markdown
Member

I’m not particular sure about this, strftime is more obvious and (subjectively) easier to read. The f-string format can be beneficial if the formatted date string is going into another f-string (as in some cases here) but not when it’s on its own.

@uranusjr Reduced to being part of larger f-strings.

I really like conciseness of the way it's done now. It very nicely focuses the concerns (date + format) in one place within the larger f-string. @uranusjr -> is that good for you as well?

Comment thread airflow/providers/elasticsearch/log/es_task_handler.py Outdated
Comment on lines 40 to 41

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It’s probably better to just convert these to literals with a comment, going through datetime just to convert into a datetime is a bit over the top.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed it to a more explicit version.

@potiuk
potiuk merged commit 94f70d8 into apache:main Aug 23, 2023
@eumiro
eumiro deleted the no-strftime branch August 23, 2023 20:13
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 2, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 2, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants