-
-
Notifications
You must be signed in to change notification settings - Fork 78
Support large args #1385
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
base: main
Are you sure you want to change the base?
Support large args #1385
Changes from all commits
4e2e05d
e03034c
e1bdaed
374dc70
ef3e702
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| from typing_extensions import Literal | ||
|
|
||
| from procrastinate import types | ||
| from procrastinate.utils import ellipsize_middle | ||
|
|
||
| if TYPE_CHECKING: | ||
| from procrastinate import manager | ||
|
|
@@ -136,7 +137,8 @@ def evolve(self, **kwargs: Any) -> Job: | |
| @cached_property | ||
| def call_string(self): | ||
| kwargs_string = ", ".join( | ||
| f"{key}={value!r}" for key, value in self.task_kwargs.items() | ||
| f"{key}={ellipsize_middle(repr(value))}" | ||
| for key, value in self.task_kwargs.items() | ||
|
Comment on lines
+140
to
+141
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you left out the part where we allow people to override this. It might be ok in this PR, but I'd like to make sure we're fine with this before a release. Doesn't have to be something gigantic, but this may be ruining someone's workflow with no hopes of improvement.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I just saw your comment in the PR body ! I'll try to think about it. It's ok to leave it out of the PR for now if it's complicated.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's easier if it can be configured in the tasks, then ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually made me think... Perhaps a better solution to all of this would be to have a way to simply exclude an argument from display ( This seems much simpler than all of this. What do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, it seemed much simpler but:
|
||
| ) | ||
| return f"{self.task_name}[{self.id}]({kwargs_string})" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -329,3 +329,17 @@ def queues_display(queues: Iterable[str] | None) -> str: | |
| return f"queues {', '.join(queues)}" | ||
| else: | ||
| return "all queues" | ||
|
|
||
|
|
||
| def ellipsize_middle( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this function, and it seems like the perfect candidate for a In particular, I like that this implementation works well around 100 chars (if we look at what happens char by char, there's no moment where we add 1 char and we end up with ellipsis and with the whole string being less than 100 chars). That said, I felt the need to test it to be 100% sure of that, so it would be awesome if the tests were included :D I specifically liked a lot that all the important configuration is passed as parameters.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I know, I'm also annoyed by the counterproductive ellipses in some implementations, hehe. Will add tests. |
||
| value: str, max_length: int = 100, suffix_length: int = 20, ellipsis: str = "..." | ||
| ) -> str: | ||
| """ | ||
| Limits the length of a string to `max_length` by placing `ellipsis` in the middle, preserving `suffix_length` at the end. | ||
| """ | ||
| prefix_length = max_length - len(ellipsis) - suffix_length | ||
|
|
||
| if len(value) > max_length: | ||
| return value[:prefix_length] + ellipsis + value[-suffix_length:] | ||
| else: | ||
| return value | ||
Uh oh!
There was an error while loading. Please reload this page.