fix(config): normalize Config.plugins entries with clear errors (#6440)#6445
fix(config): normalize Config.plugins entries with clear errors (#6440)#6445ChinmayShringi wants to merge 2 commits intoreflex-dev:mainfrom
Conversation
…6440) Passing a Plugin class instead of an instance to rx.Config(plugins=...) previously failed deep in the compiler with: TypeError: Plugin.get_stylesheet_paths() missing 1 required positional argument: 'self' Now Config._normalize_plugins() runs in _post_init and: - auto-instantiates bare Plugin subclasses (matches disable_plugins semantics), so plugins=[SitemapPlugin] behaves like plugins=[SitemapPlugin()] - raises ConfigError naming the offending entry for non-Plugin values - raises ConfigError naming the class for Plugin subclasses whose __init__ requires arguments Closes reflex-dev#6440
Greptile SummaryAdds Confidence Score: 4/5Safe to merge; the fix is well-scoped and the only concern is a P2 edge case in error messaging. Only P2 findings present — broad TypeError catch could produce a misleading error message in rare internal-TypeError scenarios, but the chained cause preserves the real error. No logic errors, no security issues, tests cover all new branches. No files require special attention beyond the minor TypeError handling note in config.py. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_post_init called] --> B[update_from_env]
B --> C[_normalize_plugins]
C --> D{For each entry in plugins}
D --> E{isinstance entry Plugin?}
E -->|yes| F[keep as-is]
E -->|no| G{issubclass entry Plugin?}
G -->|yes| H[call entry]
H -->|success| I[auto-instantiated instance]
H -->|TypeError| J[raise ConfigError: requires constructor args]
G -->|no| K[raise ConfigError: not a Plugin entry]
F --> L[append to normalized]
I --> L
L --> D
D -->|done| M[self.plugins = normalized]
M --> N[_normalize_disable_plugins]
N --> O[_add_builtin_plugins]
Reviews (1): Last reviewed commit: "fix(config): normalize plugins entries w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| normalized.append(entry()) | ||
| except TypeError as exc: | ||
| msg = ( | ||
| f"reflex.Config.plugins entry {entry.__name__!r} is a Plugin " | ||
| f"class that requires constructor arguments; pass an instance " | ||
| f"instead, e.g. plugins=[{entry.__name__}(...)]." | ||
| ) | ||
| raise ConfigError(msg) from exc |
There was a problem hiding this comment.
TypeError catch is too broad
TypeError can be raised inside a Plugin's __init__ for reasons unrelated to missing arguments (e.g. an internal type mismatch). When that happens the user sees "requires constructor arguments; pass an instance instead" even though the real failure is a bug inside __init__. The from exc chain surfaces the real cause, but the primary message is misleading.
A tighter guard would inspect inspect.signature before calling the constructor:
import inspect
elif isinstance(entry, type) and issubclass(entry, Plugin):
sig = inspect.signature(entry.__init__)
required = [
p for p in list(sig.parameters.values())[1:] # skip 'self'
if p.default is inspect.Parameter.empty
and p.kind not in (p.VAR_POSITIONAL, p.VAR_KEYWORD)
]
if required:
msg = (
f"reflex.Config.plugins entry {entry.__name__!r} is a Plugin "
f"class that requires constructor arguments; pass an instance "
f"instead, e.g. plugins=[{entry.__name__}(...)]."
)
raise ConfigError(msg)
normalized.append(entry())This raises ConfigError before attempting construction, so any unrelated TypeError from inside __init__ propagates as-is rather than being swallowed into the misleading message.
| f"reflex.Config.plugins entry {entry.__name__!r} is a Plugin " | ||
| f"class that requires constructor arguments; pass an instance " | ||
| f"instead, e.g. plugins=[{entry.__name__}(...)]." |
There was a problem hiding this comment.
instead of saying "class that requires constructor arguments;", just mention that the plugin could not be instantiated and may require arguments.
i dont really like the complex suggestion from greptile
There was a problem hiding this comment.
Done in 75a2159, reworded to "could not be instantiated and may require arguments" so we don't pre-diagnose the cause when the underlying TypeError could be from something else inside init. Skipped the inspect.signature suggestion as you preferred.
| class TestPluginsNormalization: | ||
| """Tests for the plugins config option normalization (issue #6440).""" |
There was a problem hiding this comment.
in this repo, we don't structure pytest cases in classes that don't have any shared fixtures or data. just define the test functions here at the module level.
There was a problem hiding this comment.
Done in 75a2159, flattened TestPluginsNormalization into four module-level test functions. The class had no shared fixtures or data, so it was just unnecessary nesting.
Merging this PR will not alter performance
Comparing 
Footnotes
|
- Reword TypeError message to be less prescriptive about cause - Flatten TestPluginsNormalization class into module-level test functions per repo convention
2 participants
All Submissions:
Type of change
Changes To Core Features:
closes #6440
Description
Passing a
Pluginclass instead of an instance torx.Config(plugins=...)previously failed deep in the compiler with:This PR adds
Config._normalize_plugins()(mirroring the existing_normalize_disable_plugins()) that runs in_post_initand:Pluginsubclasses soplugins=[SitemapPlugin]behaves likeplugins=[SitemapPlugin()]. This matches the lenient handling already in place fordisable_plugins.ConfigErrornaming the offending value for non-Pluginentries (e.g. a stray string), instead of failing later with a confusing stylesheet-compilation traceback.ConfigErrornaming the class when aPluginsubclass's__init__requires arguments, so the user knows to pass an instance.The issue author proposed two options; I went with option 2 (auto-instantiate) since that is already the semantic of
disable_pluginsand avoids breaking ergonomic call sites that omit().Repro before/after
Before:
TypeError: Plugin.get_stylesheet_paths() missing 1 required positional argument: 'self'deep insidecompiler/_compile_root_stylesheet.After: Config builds successfully with both plugins instantiated.
Test plan
uv run pytest tests/units/test_config.py(82 passed, 4 new cases underTestPluginsNormalization)uv run ruff check packages/reflex-base/src/reflex_base/config.py tests/units/test_config.pyuv run pyright packages/reflex-base/src/reflex_base/config.py tests/units/test_config.py(0 errors)New tests cover:
ConfigErrormentioningreflex.Config.plugins.ConfigErrornaming the class.