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

Refactor ScopeStack #9158

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Refactor ScopeStack #9158

merged 1 commit into from
Aug 30, 2023

Conversation

jjonescz
Copy link
Member

Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Aug 24, 2023
@jjonescz jjonescz marked this pull request as ready for review August 24, 2023 14:25
@jjonescz jjonescz requested a review from a team as a code owner August 24, 2023 14:25
Comment on lines -48 to +52
{
var currentScope = new ScopeEntry("__template", ScopeKind.Template);
_stack.Push(currentScope);
public void OpenTemplateScope(CodeRenderingContext context) => OpenScope(context);

// Templates always get a lambda scope, because they are defined as a lambda.
OffsetBuilderVarNumber(1);
currentScope.LambdaScope = context.CodeWriter.BuildLambda(BuilderVarName);
private void OpenScope(CodeRenderingContext context)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to have both of these methods now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, OpenScope is private. OpenComponentScope and OpenTemplateScope are two public
methods, each does a slightly different thing, the shared code is in the private OpenScope.
That seems fine to me, or what do you suggest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we wouldn't just make OpenScope internal, and delete OpenTemplateScope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason could be that OpenTemplateScope is more descriptive. So there are currently two kinds of scopes - component and template. The methods as defined now perhaps tell that story more clearly than what you suggest.

But I don't know, I'm okay doing what you say if @chsienki agrees

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I originally left it that was as it seemed more descriptive, but I'm fine just having OpenScope given that the template scope doesn't actually do anything different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@333fred what do you think? Can we leave this as it's more descriptive or you think it's better to have just OpenScope?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel too strongly. I'll approve and you can decide.

@jjonescz jjonescz requested a review from 333fred August 25, 2023 07:59
@jjonescz jjonescz merged commit f3fac03 into dotnet:main Aug 30, 2023
@jjonescz jjonescz deleted the scope-stack branch August 30, 2023 07:09
@ghost ghost added this to the Next milestone Aug 30, 2023
@jjonescz jjonescz modified the milestones: Next, 17.8 P3 Aug 30, 2023
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants