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 ability to use timedelta and stings to start_time and end_time. #8348

Merged
merged 20 commits into from Mar 26, 2024

Conversation

kajarenc
Copy link
Collaborator

@kajarenc kajarenc commented Mar 21, 2024

Describe your changes

Add the ability to use time delta and stings to start_time and end_time.

Also moved ttl_to_seconds function out of cache_utils to runtime_utils and renamed it to duration_to_seconds, since now it is also used outside of the cache module.

GitHub Issue Link (if applicable)

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python) Added!
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Copy link
Collaborator

@snehankekre snehankekre left a comment

Choose a reason for hiding this comment

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

LGTM 👍

lib/streamlit/runtime/runtime_util.py Dismissed Show dismissed Hide dismissed
lib/streamlit/runtime/runtime_util.py Dismissed Show dismissed Hide dismissed
@kajarenc kajarenc marked this pull request as ready for review March 21, 2024 17:58
Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines +91 to +94
ttl: float | timedelta | str | None, *, coerce_none_to_inf: bool = True
) -> float | None:
"""
Convert a ttl value to a float representing "number of seconds".
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we rename ttl here to duration to better match the new function name?

Suggested change
ttl: float | timedelta | str | None, *, coerce_none_to_inf: bool = True
) -> float | None:
"""
Convert a ttl value to a float representing "number of seconds".
duration: float | timedelta | str | None, *, coerce_none_to_inf: bool = True
) -> float | None:
"""
Convert a duration value to a float representing "number of seconds".

def __init__(self, duration: str):
MarkdownFormattedException.__init__(
self,
"TTL string doesn't look right. It should be formatted as"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should it be TTL here or Duration or something more neutral?

@LukasMasuch
Copy link
Collaborator

I just saw that the fragment PR also refactors this function into a more general version:

https://github.com/streamlit/streamlit/pull/8343/files#diff-149503a6503a1f0d86aed66037fad32ee2498c9160cf5bf5ec0dc01c0559c162R64

time_to_seconds is also a good general name. Maybe we wait with this PR until the fragment is merged and use the time_to_seconds function here instead?

@kajarenc
Copy link
Collaborator Author

I just saw that the fragment PR also refactors this function into a more general version:

https://github.com/streamlit/streamlit/pull/8343/files#diff-149503a6503a1f0d86aed66037fad32ee2498c9160cf5bf5ec0dc01c0559c162R64

time_to_seconds is also a good general name. Maybe we wait with this PR until the fragment is merged and use the time_to_seconds function here instead?

@LukasMasuch discussed this with Vincent, I added todos to replace the function with time_to_seconds after fragment PR merge.

@kajarenc kajarenc merged commit 8121928 into develop Mar 26, 2024
35 of 36 checks passed
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.

None yet

3 participants