fix: resolve TypeError when passing multi-events via config_expr in @trigger#3081
fix: resolve TypeError when passing multi-events via config_expr in @trigger#3081Satyamgupta2365 wants to merge 5 commits into
Conversation
…into fix/sort-lazy-plugin-commands
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge — the bug fix is correct and the only other change is a benign sort. The core fix (dict shallow copy) correctly resolves the TypeError for all current code paths. No new P0 or P1 issues were identified beyond concerns already addressed in prior review threads. The unrelated sorting change is harmless. All remaining observations are P2 or below. No files require special attention. Important Files Changed
Greploops — Automatically fix all review issues by running Reviews (3): Last reviewed commit: "Merge branch 'master' into fix/sort-lazy..." | Re-trigger Greptile |
| if is_stringish(event): | ||
| return {"name": str(event)} | ||
| elif isinstance(event, dict): | ||
| event = dict(event) |
There was a problem hiding this comment.
Shallow copy may not be sufficient for deeply-nested
ConfigValue parameters
dict(event) creates a shallow copy — the top-level dict becomes mutable, but any nested ConfigValue objects (e.g. the value of event["parameters"]) remain as ConfigValue instances. In process_parameters, isinstance(parameters, dict) will be True for a ConfigValue (since it inherits from dict), and iterating over it works fine, so the immediate bug is resolved. However, for better defensive programming, consider using ConfigValue.to_dict() (which performs a full recursive conversion to plain dicts) rather than a shallow dict() copy:
elif isinstance(event, dict):
# Deep copy to allow mutation (handles immutable ConfigValue from config_expr)
event = event.to_dict() if hasattr(event, "to_dict") else dict(event)This is a suggestion rather than a blocking issue — the shallow copy is sufficient for all current code paths.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| subgroup = self._lazy_load(source_name, source) | ||
| base.extend(subgroup.list_commands(ctx)) | ||
| return base | ||
| return sorted(base) |
There was a problem hiding this comment.
Unrelated change bundled with the
@trigger bug fix
This change (sorting the list_commands output in LazyPluginCommandCollection) is unrelated to the TypeError: ConfigValue is immutable bug described in the PR. It has no connection to the @trigger decorator or config_expr.
While the change itself is benign and reasonable (producing deterministic, alphabetically-ordered CLI command listings), bundling it with an unrelated bug fix makes the PR harder to review and bisect. Consider splitting this into a separate PR or adding a note in the PR description explaining why this change is included.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
This PR fixes a bug where using
config_exprto define multiple events in the@triggerdecorator resulted in aTypeError: ConfigValue is immutablewhen trying to set event parameters.Context
When
@trigger(events=config_expr(...))is used, the configuration values are returned asConfigValueobjects. These objects are immutable and act as wrappers around the underlying dictionary to prevent accidental modifications.The
TriggerDecorator.process_eventmethod, however, expects a mutable object because it attempts to setevent["parameters"]directly. This leads to the following traceback: