Accept search attributes for dev server #562
Conversation
|
There is an issue with the dev server using search attributes, currently blocked on temporalio/temporal#6195. Leaving as draft for now. |
985fff0 to
e31a280
Compare
|
Now that CLI has been released with newer server, this is now ready for review |
temporalio/testing/_workflow.py
Outdated
| @@ -91,6 +91,7 @@ async def start_local( | |||
| download_dest_dir: Optional[str] = None, | |||
| ui: bool = False, | |||
| runtime: Optional[temporalio.runtime.Runtime] = None, | |||
| search_attributes: Sequence[temporalio.common.SearchAttributeKey] = [], | |||
There was a problem hiding this comment.
This and existing parameters are violating no-mutable-defaults.
There was a problem hiding this comment.
I am not aware of this rule on our codebase, is it new? We don't mutate this parameter but Python is not expressive enough to mark a list as immutable. It is an immutable default despite Python's lack of expressiveness. I wouldn't expect such a rule (even if it's not smart enough to know this isn't a mutable default) to affect whether the PR is approved.
There was a problem hiding this comment.
It's a standard rule in Python that default argument values shouldn't be mutable, e.g.
https://google.github.io/styleguide/pyguide.html#2124-decision
https://docs.astral.sh/ruff/rules/mutable-argument-default/
We can fix the existing examples in a separate PR, but let's not add more violations in this PR.
There was a problem hiding this comment.
I disagree, I think we should be consistent with what's there and not institute new rules after PRs are opened. We can change the consistency in one fell swoop, but don't need to hold up new PRs trying to be consistent I don't think. This is brand new news that we are now blocking PRs for these rules.
There was a problem hiding this comment.
OK, I mean, you'd only have to change it from [] to () 😄 But I agree we don't want to confuse this PR with purging the codebase of all existing violations. I've never worked in a Python codebase that doesn't enforce that rule. Opened #762
There was a problem hiding this comment.
Ah, that is simple enough, I will make that change. I do wish the rule was to not mutate a parameter with a mutable default instead of just not having a mutable default, but unfortunately Python and its tooling can't really check that well.
What was changed
WorkflowEnvironment.start_localnow accepts asearch_attributesargument that leverages Accept search attributes for dev server and disable cache by default cli#593Checklist