Skip to content
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

Parent context refactor #24

Merged
merged 8 commits into from
Mar 3, 2024
Merged

Parent context refactor #24

merged 8 commits into from
Mar 3, 2024

Conversation

tbrlpld
Copy link
Owner

@tbrlpld tbrlpld commented Feb 17, 2024

Description

This PR refactors how parent_context is handled between the render_html and get_context_data methods.

Previously, the render_html method was accepting the parent_context to be optional, defaulting to None. If the parent_context is None it is overridden to be an empty Context object. That context object is then used to call get_context_data.

Usually, the render_html method is called from the component template tag. The template tag always passes a Context object as the parent_context. It never passes None.

The get_context_data method required a parent_context to be passed, it does not have a default value for the argument. But, the base implementation in Component.get_context_data just ignores the parent_context object and returns an empty dict.

This setup has the effect that when child classes override the get_context_data that their
super().get_context_data(parent_context) call require the parent_context argument. But they only get an empty dict. This seems confusing and unnecessary, since we ignore the parent_context anyways. The only reason it's there is so that child components can make use of it.

Also, by always passing a Context object as the parent_context from render_html to the get_context_data, this suggest that this would really be a parent context, which typically includes things like the request object. However, we cannot rely on the request to be in the parent_context because render_html might have been called with None and just gives us an empty Context.

This inconsistency between the parent_context that is passed to render_html vs the one passed to get_context_data could easily lead to confusion in the case where we call render_html directly and don't pass a parent_context (as opposed it being called by the component tag which always passes the parent context).

This PR tries to make the parent_context between render_html and get_context_data more consistent. To do so, the parent_context argument of get_context_data now defaults to None. We still just return an empty dict. We don't override the parent_context in the render_html method anymore.

This means that all decisions about the parent_context can now be made in the get_context_data method. This feels like the appropriate place for all handling about context data, as the method is accordingly named.

The only case where this refactor would require a change in usage (of which there is not much at this point) is when the render_html method is called directly without passing a parent context and get_context_data expects to pull some data out of the parent_context. This case would need to guard against the parent_context being None.

I have checked the Wagtail code base and found no usage conflicting with this change. The Wagtail test suite also passes just fine. Therefore, I consider this change as "Wagtail-compatible", which is the only "backward-compatibility" I am concerned with at this stage.

Checklist

  • CHANGELOG.md has been updated.
  • README.md has been updated.
  • Checked Wagtail compatibility.
  • Self code reviewed.

This does seem like a weird place to deal with it. There is a special
method (`get_context_data`) to handle the context generation. If we need
some special handling for a `None` parent context, it should go there.
Copy link

codecov bot commented Feb 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d9fe305) to head (9e4edba).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #24   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          255       254    -1     
=========================================
- Hits           255       254    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tbrlpld tbrlpld self-assigned this Feb 17, 2024
@tbrlpld tbrlpld enabled auto-merge March 3, 2024 04:26
@tbrlpld tbrlpld merged commit bd265d4 into main Mar 3, 2024
8 checks passed
@tbrlpld tbrlpld deleted the parent-context-refactor branch March 3, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant