Implement .collectWithParameters() and .collectAllParameters()

[andrewboni]

This PR implements .collectWithParameters() and .collectAllParameters()

[dontgitit]

Nice, this looks a lot more comprehensive than collect/collectReferenceParameters (and in fact would probably be trivial to re-implement those two by using this)

[jknack]

Guys, can you explain why do we need this? why existing implementation doesn't work?

Also, I do a commit with a .jar file?

Thanks

[dontgitit]

Hey @jknack , we need this because we'd like to essentially verify some things before we render the Template. collect gets us part of the way there (gives us the Tags), but doesn't tell us which Parameters they're invoked with. collectReferenceParameters gives us (only the reference) parameters, but there's no way to tell which helper/tag they're called from.

In short: We added a new helper, {{myHelper param1 param2}}. We want to perform some validation on the compiled Template. For example, if myHelper is used, we want to verify that param1 is a StrParam; otherwise, we want to reject the template. {{myHelper "param1"}} is valid, {{myHelper param1}} is not valid (even if param1 would evaluate to a string during rendering).

[dontgitit]

And no, we added the built jar to use in our own project until this makes its way into one of your builds; certainly do not commit the .jar to master.

[jknack]

can we add params to existing Tag class so we don't introduce a new TagWithParams class?

Jar has been added to the pull with this commit. I can't merge this pull with the jar there

[hjz]

Removed the jar

[andrewboni]

@jknack not sure I follow; can you clarify? There doesn't seem to be an existing Tag class?

[codecov-io]

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@8b46b71). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #637   +/-   ##
=========================================
  Coverage          ?   87.21%           
  Complexity        ?      863           
=========================================
  Files             ?       79           
  Lines             ?     3089           
  Branches          ?      418           
=========================================
  Hits              ?     2694           
  Misses            ?      268           
  Partials          ?      127

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8b46b71...e0f7722. Read the comment docs.

[coveralls]

Coverage decreased (-0.04%) to 91.324% when pulling 1cd3eacd9da83d08146051a2b13d887e5774cf35 on Iterable:master into 219f338188029400ccfba4149ffaaf87d79da166 on jknack:master.

[andrewboni]

@jknack gentle bump; just want to make sure we're on the same page before writing more tests 👍

[jknack]

I thought there was a Tag class already, but was wrong. Your pull looks good, we should do these changes too:

• Rename TagWithParams to Tag
• Move Tag to public package: com.github.jknack.handlebars
• This is all about metadata so Tag should have a List<String> getParams (not List<Param> getParams). Param is an internal class and doesn't carry the parameter name.

With these changes a call to collect(...) produces a full metadata template and collectReferenceParameters can be marked as deprecated.

Cool?

[dontgitit]

@jknack Param is an internal class, but maybe we should expose it, rather than changing it to List<String> getParams?

Reasons: 1.) Param encodes important metadata, such as the parameter type (literal Str and Def, or reference Ref and Var) 2.) how would you translate a non-literal Param into a String? StrParam is obvious; DefParam is somewhat straightforward (get the value and maybe .toString).... but what would getParams return for a VarParam or RefParam? it's a little ambiguous whether it should just return the string expression for the parameter name, or a toString of the value of the param (which is not even possible before rendering the template because there is no context). Exposing Param would allow the user a much better level of control/info.

[jknack]

Good stuff, let's make Param a public class.

[dontgitit]

hey @jknack , finally got around to making the changes you suggested! let me know how it looks...

I marked collect as deprecated rather than collectReferenceParameters, since collect does pretty much the same as the new method, but collectReferenceParameters is a little bit different? What do you think? I do think we can reimplement it by using collectWithParameters though...

[dontgitit]

hey @jknack , wondering if you have any other thoughts/concerns about this PR, or if we could move it along somehow? thanks!

[dontgitit]

P.S. I think since our last discussion, there's been an additional feature added - "internal data". This is so that special data can be passed along, that's not visible/accessible from the user's perspective, but helpers can access it. For instance, if a helper needs access to some special data called secret, we can now put it in internal storage rather than public data/context, so that it's not viewable via {{secret}}

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 82.05128% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 87.19%. Comparing base (1d44be3) to head (e0f7722). Report is 95 commits behind head on master.

:exclamation: Current head e0f7722 differs from pull request most recent head 888f789. Consider uploading reports for the commit 888f789 to get more accurate results

Files	Patch %	Lines
...hub/jknack/handlebars/internal/HelperResolver.java	66.66%	2 Missing and 2 partials :warning:
...ithub/jknack/handlebars/internal/BaseTemplate.java	70.00%	2 Missing and 1 partial :warning:
...ain/java/com/github/jknack/handlebars/Context.java	83.33%	2 Missing :warning:
...rc/main/java/com/github/jknack/handlebars/Tag.java	80.00%	2 Missing :warning:
...a/com/github/jknack/handlebars/internal/Block.java	87.50%	0 Missing and 2 partials :warning:
...jknack/handlebars/internal/ForwardingTemplate.java	50.00%	1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #637      +/-   ##
============================================
- Coverage     87.30%   87.19%   -0.11%     
- Complexity      862      868       +6     
============================================
  Files            79       79              
  Lines          3079     3101      +22     
  Branches        437      419      -18     
============================================
+ Hits           2688     2704      +16     
- Misses          263      270       +7     
+ Partials        128      127       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.