Merging fork into trunk

[oformaniuk]

Should replace /pull/352

Requires some more changes before merging.

What's inside (inspired by /issues/294):

• fixes to multiple Github issues:
  • BlockParams support (closes Sequence contains more than one element #301)
  • Support returning objects from helpers (closes Support for returning a complex object from a subexpression #264)
  • Lookup helper (closes Cannot compile template with Lookup helper - signature or security transparency is not compatible #268)
  • JObject performance problems (closes RuntimeBinder Memory leaks #308)
  • Support more advanced helper construction (closes Question: How to create a custom helper with access to @index #151, closes Question: How to create a custom helper with access to @index #151)
  • Support helper resolvers (closes [Suggestion] Fallback to instance/static method if helper cannot be resolved #336)
  • Block helper inversion (closes BlockHelper inversion (^) throws #354)
  • helperMissing and blockHelperMissing (closes Implement hooks for missing helpers #350)
  • configurable IFormatProvider (closes Specifiy CultureInfo ? #344)
  • blockHelper late binding (closes Helper not registered first time #327, closes Block Helpers cannot be late bound #194)
  • Other issues:
    • closes HandlebarsUndefinedBinding thrown for null ExpandoObject values #351
    • closes Can custom helper return object? #347
    • closes Allow .length in addition to .Count on arrays #307
    • closes @last element in nested each refers to the parent item ( C# ) #243
    • contribution to Please report betters errors (context) when having mistakes in the template #314
• global refactoring (without breaking changes to the public API)
  • use zjklee/ExpressionShortcuts to improve maintainability of Expression Trees
• improvements to extensibility
• performance improvements (depends on template complexity):
    - 2-10x compilation time improvement 
    - 2-10x execution time improvement 
    - up to 20x less memory consumption during compilation and execution

Due to the nature and amount of changes I'm not able to extract them to a separate PRs one for each issue at this point.
I'd continue to support my fork (and back-port changes here) until the PR is merged.

[sferencik]

@zjklee I wonder if this is too big for anyone to review in full. It would make more sense to deliver it in multiple PRs. Why is that a problem, given you have the work separated into many commits?

Also, when I looked at your PR in March, some of the issues it fixes were questionable feature requests: they were asking for things that are not availbale in Handlebars.js, and there was some reluctance to diverge functionally from Handlabars.js.

[oformaniuk]

@sferencik Hi, thanks for your attention.

I wonder if this is too big for anyone to review in full. It would make more sense to deliver it in multiple PRs. Why is that a problem, given you have the work separated into many commits?

I clearly understand your concern about the size of the PR and I do not expect it to be reviewed in detail. Under other circumstances I'd definitely prefer going with smaller PRs but assuming code-base is too diverged from the trunk I feel the effort of splitting everything to a smaller meaningful portions would be too high even if I have more or less structured parts in context of my fork.

We've discussed the situation with @rexm recently and decided to merge my changes to the trunk. However I still want to make some changes and tests before merging.
The PR is mostly for visibility purpose to collect concerns from the community in terms of functionality rather then implementation (the exact think you did 👍 ).

Also, when I looked at your PR in March, some of the issues it fixes were questionable feature requests: they were asking for things that are not available in Handlebars.js, and there was some reluctance to diverge functionally from Handlabars.js.

Can you please elaborate more on this if you have any concrete concerns?
Most of the questionable things have been added as an extension point or hidden behind a feature toggle so should not affect default experience. Here's a list of differences from Handlabarsjs I can come up with:

• support @last for object iterators: hidden behind feature flag [default: false] (Configuration.Compatibility.SupportLastInObjectIterations);
• aliases (Allow .length in addition to .Count on arrays #307): done via extension point;
• relaxed helper naming (Backward compatibility with Handlebars.Net in terms of helper naming oformaniuk/Handlebars.Net#32): hidden behind feature flag [default: false] (Configuration.Compatibility.RelaxedHelperNaming);
  This one is more about backward compatibility with current version. Default behavior corresponds to Handlabarsjs.
• helper resolvers ([Suggestion] Fallback to instance/static method if helper cannot be resolved #336): done via extension point;
• may be some semantic differences in terms of implementation details but not affecting feature parity;

Other then these few differences changes looks aligned with Handlabarsjs. I'd be happy if you can point at other things that are not available in Handlabarsjs but may be merged to Handlabar.Net.

[sferencik]

We don't have a strong/critical dependency on Handlebars.Net but if we did, I have to admit I would be quite uneasy about the massive change going into the codebase without a proper community review. I think it's a pity you're not willing to break this down to smaller chunks which the maintainers could consider & review in isolation.

The changes you've made, the feature requests, the bugs fixed, the infrastructure improvements, it all sounds fantastic. You've obviously put a lot of effort into it, and the project will be the better for it. However, there's always great potential for improvement in a code review, both in terms of coding/design and - much more importantly - in the discussion about the functionality. You're bypassing this potential, for the detriment of your own work, and of the whole project. Code review may also catch bugs which, with all due respect to your work, you're at risk of introducing with this amount of work. All these benefits of code review apply even if you're a brilliant engineer with a very high standard of work (which I'm sure you are!).

@rexm's offer to accept the PR is generous. I think the onus should be on you to break this down at least a little. Your argument that the "code-base is too diverged from the trunk" is cyclical: if you broke things down, the code-base wouldn't be too diverged. Surely your first commit applies on top of the current trunk? It even seems that you created a feature branch for each logical change yourself, which makes me wonder why we can't just replay those one by one as individual PRs. I imagine a string of smaller PRs could be reviewed in quick succession. Plus, the all-or-nothing attitude puts the community in a corner, which I find a little untoward. But anyway, this is for @rexm to decide, I'm just voicing my opinion.

As for the questionable features, thanks for listing the ones you're aware of. I don't remember the details of the ones I came across in March, which I'm sorry about. Thanks for taking care to hide them behind feature toggles.

All in all, your work deserves appreciation. I just wanted to share my perspective, and build a case for a proper peer review.

[rexm]

From my perspective, there are only two practical solutions given where we are at: we could go back in time on the fork and submit a PR from each major working milestone from then up through now, in succession; or evaluate the entire delta in one large commit. Both have advantages and complications. I think asking @zjklee to decompose the current delta into logically scoped, bite-sized, non-breaking PRs is not practical or the best use of his time. In the aggregate, the same amount of change has to get reviewed one way or another. For those of us who care about the project, we can work together to do things like using our heuristics to ask deeper questions about areas that catch our attention; use this thread to divide and conquer.

One specific suggestion: let’s set a target merge date and figure out how to back into it.

[oformaniuk]

@sferencik

However, there's always great potential for improvement in a code review, both in terms of coding/design and - much more importantly - in the discussion about the functionality. You're bypassing this potential, for the detriment of your own work, and of the whole project. Code review may also catch bugs which, with all due respect to your work, you're at risk of introducing with this amount of work. All these benefits of code review apply even if you're a brilliant engineer with a very high standard of work (which I'm sure you are!).

Don't get me wrong thinking I underestimate the value of code review and do not want my code to be reviewed before merging. By saying "I do not expect it to be reviewed in detail" I mean "Assuming the amount of changes I understand that it's hard to find time to review it line-by-line".
Generally I'd prefer having a code review rather then not. I'd love GitHub allow stacking PRs one on top of another in cross-fork environment so I could create a bunch of stacked PR to the trunk from the very beginning. Unfortunately this is possible only inside a single fork. So I had to go my way working at my fork as my first PR had no attention.

I think it's a pity you're not willing to break this down to smaller chunks which the maintainers could consider & review in isolation.

It even seems that you created a feature branch for each logical change yourself, which makes me wonder why we can't just replay those one by one as individual PRs.

At this point in time it would introduce more complications as changes were made iteratively and oftenly touching same places. Breaking existing git history into separate PRs would require reviewing the same place again and again. Also, why should someone review code that already had been changed in next set of PRs?
From the other hand it's not possible (or at least to complicated) to break the head at smaller meaningful pieces due to the code-base differences.

I think the onus should be on you to break this down at least a little.

I think this is achievable. I think that it would be better to reset my master to tag 1.3.8 and deliver changes that came after it as separate PR(s). However it would be required to merge one more PR (removing dependency on Microsoft.Extensions.ObjectPool) on top of it before creating new Handlebars.Net package.
In addition extract changes not related to Core library (like extensions, CI, etc.) into separate PRs, at least with the write access to the trunk I can stack them one on another.
This should decrease the amount of changes in this particular PR.

@rexm

One specific suggestion: let’s set a target merge date and figure out how to back into it.

I would need some time to make a breakdown I've described above. I think by the end of the week I'd be able to mark PR as "Ready for review". I like the idea of target merge date. From my side I'd try my best at providing fast answers to the comments.

[sferencik]

Hi guys, thanks for your input, and thanks @zjklee for your willingness to break it down a little. I'll try to have a look myself when these are ready. Good luck!

[oformaniuk]

@rexm , @sferencik any progress on reviewing the PR?
I'd like to come back to @rexm's suggestion on setting target merge date. Assuming that the PR is in "ready for review" state for more then two weeks now I suppose two more weeks is a doable target.
Any objections or suggestions?

[sferencik]

@zjklee I've reviewed the code very briefly. I like your changes, but haven't gone into much detail. I especially like the fixes and new features as showcased by the test suite. I like the new returning helpers. I appreciate you incorporated my late-bound-helper-trumps-variable change from #350. I see you've done a ton of good refactoring & clean up.

What I haven't looked at in too much detail is the actual code changes in the guts of the library. I think someone like @rexm is in a much better position for that, and will have more useful insights.

Thanks again.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

81.7% Coverage
1.7% Duplication

[rjendoubi]

Super excited to see this land in master 🎉 Many thanks to @zjklee for the huge work, and the other maintainers for reviewing 🙏

Given the latest release was only a few days ago, with the previous one being months before that, is there likely to be a new release imminently, or would one be best advised to build from master for now if keen to take advantage of these new features?

Thanks again!

[oformaniuk]

Hi @rjendoubi.
Previous release is the last 1.x release with all changes that were made after 1.10.1.
As for the next release - it may take some time as I have some more changes planned and some of them would not be compatible with the APIs introduced in this PR. Most of them are already in my fork but before merging I'd like to revisit some of them. Good news is that the change set is smaller then this one and it would take less time.