-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: Avoid downcasting of text-enabled child plugins if plugins are already downcasted #58
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue where text-enabled child plugins were being unnecessarily downcasted, even when they were already downcasted. This was causing issues with rendering and editing of child plugins. The changes ensure that downcasting is only performed when necessary, improving the overall stability and performance of the text plugin. Sequence diagram for plugin rendering with optimized downcastingsequenceDiagram
participant Editor
participant TextPlugin
participant PluginRenderer
participant ChildPlugins
Editor->>TextPlugin: Request render plugin
TextPlugin->>PluginRenderer: plugin_tags_to_html()
alt Child plugins already downcasted
PluginRenderer->>ChildPlugins: Use existing instances
else Child plugins not downcasted
PluginRenderer->>ChildPlugins: Get plugins from text
ChildPlugins-->>PluginRenderer: Return downcasted plugins
end
PluginRenderer-->>TextPlugin: Return rendered HTML
TextPlugin-->>Editor: Display rendered content
Class diagram showing updated function signaturesclassDiagram
class utils {
+_render_cms_plugin(plugin: CMSPlugin, context)
+random_comment_exempt(view_func: callable): callable
+plugin_to_tag(obj: CMSPlugin, content: str, admin: bool)
+_plugin_tags_to_html(text: str, output_func: callable, child_plugin_instances: Optional[list[CMSPlugin]]): str
+plugin_tags_to_user_html(text: str, context: Context, child_plugin_instances: list[CMSPlugin]): str
+plugin_tags_to_admin_html(text: str, context: Context, child_plugin_instances: list[CMSPlugin]): str
+plugin_tags_to_db(text: str): str
+replace_plugin_tags(text: str, id_dict, regex: str): str
}
note for utils "Added type hints and new child_plugin_instances parameter"
Flow diagram of plugin rendering processflowchart TD
A[Start Render] --> B{Child plugins provided?}
B -->|Yes| C[Use existing instances]
B -->|No| D[Get plugins from text]
C --> E[Render plugins]
D --> E
E --> F[Convert to HTML]
F --> G[Return rendered content]
G --> H[End Render]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fsbraun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add a comment explaining why it's now safe to remove the browser reload call in cms.editor.js. This will help future maintainers understand the change.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
==========================================
+ Coverage 81.28% 81.40% +0.11%
==========================================
Files 17 17
Lines 935 941 +6
Branches 104 105 +1
==========================================
+ Hits 760 766 +6
Misses 132 132
Partials 43 43 ☔ View full report in Codecov by Sentry. |
…s-text into feat/db-optimization
Summary by Sourcery
Fix the downcasting of text-enabled child plugins.
Bug Fixes:
Enhancements:
Tests: