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

Rework of context handling #18

Merged
merged 4 commits into from
Dec 28, 2020
Merged

Conversation

rbeard0330
Copy link
Contributor

Hey, sorry to drop a bunch of changes on you, but I encountered an issue, had to dig in deep to figure out what was going on, and then needed to change a bunch of stuff to fix my bug. Here's an overview:

My original problem came when I tried to pass an object as a variable into a component_block tag, then have the context method process it into a handful of context variables. Long story short, this wasn't working, because the context method sometimes gets called twice, with the outputs of the first call being the inputs of the second call. That works fine if the context method doesn't transform its arguments, but fails when the context variables are different than the component params.

My fix was essentially just to have a single place for calling the context method (in the ComponentNode's render method), then to pass the context itself into the component's render method. I think this is more straightforward and fixed my problem nicely. As a side benefit, I think it will fix the CSRF problem in #12, at least when component_block is used. Previously, the non-slot portions of the component were being rendered with a bare Context, not a RequestContext. I reworked it so that the component renders itself with the context (presumably a RequestContext) that the template engine passes to it, so hopefully that will help. I think using a component tag still won't work, because that tag doesn't get a context. You could fix that by making it a takes_context tag, but I didn't make that change.

I also made some assorted tweaks as I was studying the code to help me follow better. I left those in this PR, but you can obviously disregarded them if you don't think they're helpful.

I added a bunch of tests to look at various permutations of context, and everything passes. That said, I can't swear that these changes won't break someone's code in some subtle way.

I hope these are helpful, and I'm happy to provide more explanation about anything I did.

@EmilStenstrom
Copy link
Owner

This looks great. Thanks for refactoring my code and making it clearer. Getting rid of getfullargspec is quite an accomplishment among many!

Few things before this can be merged cleanly (I really want this in, all of it!):

  • Would you mind rebasing against master? This PR continues on your last PR, so all the old commits are still there.
  • The reason CI is breaking is because of flake8. You can get it locally by running pip install flake8 and flake8 in the root of the project.
  • Minor: in test_context {%endcomponent_block %} should probably be {% endcomponent_block %} with a space.

How to fix the flake8 errors:

  • For the "line too long" errors, just add a # NOQA comment at the end of the line, they are fine.
  • Remove the double linebreak at the end
  • Remove the python 2 compatibiliy stuff that imports the now unused getfullargspec

This is great! :)

@rbeard0330
Copy link
Contributor Author

Thanks! I'll get these changes made tomorrow. Best holiday wishes!

@EmilStenstrom
Copy link
Owner

Hi again! The changes to fix the lints look great, but I'm still seeing old commits in the list above. Isn't "Add context tests" the first one in this PR?

@rbeard0330
Copy link
Contributor Author

Sorry, I'm the absolute worst at git. Hopefully that did it...

@EmilStenstrom EmilStenstrom merged commit 93b8a74 into EmilStenstrom:master Dec 28, 2020
@EmilStenstrom
Copy link
Owner

EmilStenstrom commented Dec 28, 2020

Now released as django-reusable-components 0.5 on pypi! Happy to have you onboard! 🥳

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.

None yet

2 participants