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)

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

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).

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.

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

Removed the jar

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

Codecov Report

❗ 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.

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

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

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 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?

@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.

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

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...