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

Child and local scope handling by TemplateContext #48

Merged
merged 3 commits into from
Dec 1, 2017
Merged

Child and local scope handling by TemplateContext #48

merged 3 commits into from
Dec 1, 2017

Conversation

jeremycook
Copy link
Contributor

I believe this addresses the source of variables leaking into parent scope like in issue #45. This affects the include and for statements.

One thing I did was change TemplateContext.EnterChildScope from returning a Scope to returning void. This is because the TemplateContext.ReleaseScope method should be called and not the Scope object's ReleaseScope. I want to encourage callers to use TemplateContext's Release Scope method. Calling code can still technically call the Scope object's ReleaseScope method directly by accessing it via the TemplateContext.LocalScope property, but the result of doing so is undefined behavior.

This could be further refactored by introducing an interface that the Scope class would implement. The interface would not surface the ReleaseScope method. The TemplateContext.LocalScope property would be of this type of interface instead of Scope, and further guarding calling code calling TemplateContext.LocalScope.ReleaseScope directly.

@jeremycook
Copy link
Contributor Author

I updated include tests so that they are working "as expected" now. Unfortunately there are 3 date filter tests that are failing.

@sebastienros
Copy link
Owner

Would you mind implementing your suggestion by returning an IDisposable that would call Release on the Dispose. Then remove the try/catch statements and just do using (context.EnterScope()). Keep the ReleaseScope public, but maybe rename it to LeaveScope ?

@jeremycook
Copy link
Contributor Author

Here you go. The code feels quick and dirty. I use a delegate to allow the scope to reset the TemplateContext to its parent scope on dispose. It seems like a stack could be better than these recursive Scope objects. If you had two child scopes and they were disposed out of order strange things could happen. Suggestions welcome.

@sebastienros
Copy link
Owner

You are right, we would need an AsyncStatic field in this case, just revert to the previous commit and I'll merge. Sorry to that.

@jeremycook
Copy link
Contributor Author

Should be good to go. I removed the IDisposable Scope commit.

@jeremycook
Copy link
Contributor Author

BTW what do you mean by AsyncStatic field? Is there an example of what you're thinking out in the wild?

@sebastienros
Copy link
Owner

https://github.com/aspnet/Caching/blob/dev/src/Microsoft.Extensions.Caching.Memory/CacheEntryHelper.cs#L11

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.

2 participants