Inline & strip down ExpressionShortcuts - #669
Open
TheConstructor wants to merge 2 commits into
Open
Conversation
This elemiminates all NRT-warnings in the solution
Member
|
I have no idea what any of this means:
A pull request description should describe the problem or opportunity, and explain the solution and intended impact. This is tracing the history of a change you made on an abandoned repository. Can you try again? |
Member
|
ExpressionShortcuts already inlines source code into the Handlebars package - it is distributed as source-code NuGet. |
Contributor
Author
|
Sorry for the late reply. Which build warnings are fixed by this PR?I just pulled up https://github.com/Handlebars-Net/Handlebars.Net/actions/runs/31321612216/job/93265376015?pr=668 which is the build of the last merged PR, and expanded the Build-step. The following NRT (Nullable-Reference-Type) Warnings appear because ExpressionShortcuts is not currently annotated (and some more): Long list of warningsWhen I first introduced the NRT-Annotations in the project Rider would allow me to edit the included sources from the ExpressionShortcuts-package in my local store - not very helpful. I reverted the changes to my locally cached copy of course after realizing, and created oformaniuk/ExpressionShortcuts#5 instead. I definitely hope Rider fixes the way it handles source-files contributed by NuGet-references. An issue solvedNot included in oformaniuk/ExpressionShortcuts#5 as I only realized the root-cause during the removal of stuff not needed from ExpressionShortcuts in Handlebars.Net. When I set out to write #606 I came across source/Handlebars/Compiler/Translation/Expression/PartialBinder.cs lines 36 - 94 if (decorators.Count > 0)
{
var bindingContext = CompilationContext.Args.BindingContext;
var writer = CompilationContext.Args.EncodedWriter;
var parentContext = bindingContext;
if (pex.Argument != null || partialBlockTemplate != null)
{
var value = pex.Argument != null
? Arg<object?>(FunctionBuilder.Reduce(pex.Argument, CompilationContext, out _))
: bindingContext.Property(o => o.Value);
var partialTemplate = Arg(partialBlockTemplate);
bindingContext = bindingContext.Call(o => o.CreateChildContext(value, partialTemplate));
}
var partialName = Cast<string>(pex.PartialName);
var configuration = Arg(CompilationContext.Configuration);
var isBlock = Arg(pex.IsBlock);
var indent = Arg(pex.Indent);
var templateDelegate = FunctionBuilder.Compile(
new []
{
Call(() =>
InvokePartialWithFallback(partialName, bindingContext, writer, (ICompiledHandlebarsConfiguration) configuration, isBlock, indent) // NOSONAR S1944 — ExpressionShortcuts operator; not a runtime hierarchy cast
).Expression
},
CompilationContext,
out _
);
var decorator = decorators.Compile(CompilationContext);
return Call(() => decorator.Invoke(writer, parentContext, templateDelegate))
.Call(f => f.Invoke(writer, parentContext));
}
else
{
var bindingContext = CompilationContext.Args.BindingContext;
var writer = CompilationContext.Args.EncodedWriter;
if (pex.Argument != null || partialBlockTemplate != null)
{
var value = pex.Argument != null
? Arg<object?>(FunctionBuilder.Reduce(pex.Argument, CompilationContext, out _))
: bindingContext.Property(o => o.Value);
var partialTemplate = Arg(partialBlockTemplate);
bindingContext = bindingContext.Call(o => o.CreateChildContext(value, partialTemplate));
}
var partialName = Cast<string>(pex.PartialName);
var configuration = Arg(CompilationContext.Configuration);
var isBlock = Arg(pex.IsBlock);
var indent = Arg(pex.Indent);
return Call(() =>
InvokePartialWithFallback(partialName, bindingContext, writer, (ICompiledHandlebarsConfiguration) configuration, isBlock, indent)
);
}If you look closely both branches up to Still just moving the common part above and outside the branches lead to tests failing. This is due to ExpressionShortcuts' So why did it fail?In the decorator-case the compiler decides to nest its created classes: [CompilerGenerated]
private sealed class <>c__DisplayClass7_2
{
[Nullable(0)]
public DecoratorDelegate decorator;
[Nullable(0)]
public TemplateDelegate templateDelegate;
[Nullable(0)]
public PartialBinder.<>c__DisplayClass7_0 CS$<>8__locals1;
public <>c__DisplayClass7_2()
{
base..ctor();
}
}I accounted for that in I feel like we shouldn't have code break just by moving unchanged lines one scope up. @oformaniuk if you like I can write a separate PR for that change for ExpressionShortcuts, too. Why this PR?
@rexm to address your comment: I thought getting rid of a lot of warnings would be self-explanatory, and wanted to show you why I make this your problem and not the upstream's (ExpressionShortcut's) problem, which causes these warnings. I tought you would ask otherwise @oformaniuk Did I answer your question now? I had hoped, that I addressed the question in my initial comment and the PR with your repository. Don't hesitate to ask for details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
So we now have NRT and I really like it! But currently we get some warnings during the build, mostly related to ExpressionShortcuts. I looked into it and created oformaniuk/ExpressionShortcuts#5, that would allow to get rid of the NRT-warnings.
Over a month has passed and the last change to any branch of that repository is currently 6 years old.
Now this library doesn't use everything of ExpressionShortcuts, and so I started out by adding an annotated copy of ExpressionShortcuts and then tried to strip out as many unnecessary features as possible, to not introduce a higher maintenance burden than necessary. I also tried to simplify a bunch of things - 6 years is quite some time in programming after all - and judging by the tests I may have been successful.