Interface for template engines

[mfiumara]

Initial draft for having interfaces for template engines. This can reworked and is open for discussion.

applies to #248

[ReneWerner87]

@mfiumara thanks for the effort
the interface looks good, but the usage in the first engine does not

in many cases the engines have additional functionalities, so i think we have to export them as well

for the check if the engine complies with the interface i would suggest we use "embedded interfaces" or "typechecks in test or real code".

[mfiumara]

Hi,

Thanks for the feedback I'll come up with a refinement of this draft later tonight by embedding interfaces. I see what you mean and we're indeed not exporting all functions of the engine right now.

[mfiumara]

Hey I've added an embedded interface for Pug such that all functions are exported now.

For the remainder of the engines I'd go through all of them in a similar fashion. Let me know if anything else is required before I go through with that. Perhaps I should move the function comments inside the interface definition? Or copy them over?

[efectn]

I think internal dir is not good idea. Someome else may want to use this interface in their projects. It can be template.go in the project root like how it's on storage repo

[ReneWerner87]

I think internal dir is not good idea. Someome else may want to use this interface in their projects. It can be template.go in the project root like how it's on storage repo

Yes, good point

[mfiumara]

@efectn Moved to root into template.go.

[mfiumara]

On another note I see that a lot of the functions are practically the same for all Engines. If we could make some or all of the fields of the Engine struct public we could also embed the struct and define most of the methods once for all of the engines. It will save a lot of code and will be easier to add new engines.

Let me know if that's a good idea or not, it does mean the members will become public which perhaps is not what we want.

[efectn]

On another note I see that a lot of the functions are practically the same for all Engines. If we could make some or all of the fields of the Engine struct public we could also embed the struct and define most of the methods once for all of the engines. It will save a lot of code and will be easier to add new engines.

Let me know if that's a good idea or not, it does mean the members will become public which perhaps is not what we want.

Agree with you. It's worth to try

[mfiumara]

@efectn I've done the following:

• Added struct definition into internal: this should not be used by external packages as far as I understand
• The interface definition remains in the root (template.go) to be used by others
• Individual engines can now embed the internal struct to have all basic methods promoted into their own interface type

I don't see the commit here yet in the PR, I'm not too familiar with Github but perhaps that takes some time to appear.

[mfiumara]

Added second Engine Mustache to illustrate further. I should be able to do the rest fairly quickly let me know if this is the way forward.

[mfiumara]

I think I'm done @efectn. Would be great if you can have a look.

I additionally want to remove all redundant tests and move them to internal, that should save some unnecessary tests.

[efectn]

Can you check hound comments @mfiumara

[mfiumara]

@efectn Done

[efectn]

@mfiumara can you resolve the conflicts, i removed parse() from engines

[mfiumara]

@efectn Fixed conflicts, rebased and squashed commits.

[mfiumara]

Hi @efectn could this be merged? I had to resolve conflicts again due to other changes being merged.

[efectn]

Hi @efectn could this be merged? I had to resolve conflicts again due to other changes being merged.

The changes are OK to me but we should wait for one more review. @ReneWerner87 will review the PR when he is free.

[ReneWerner87]

@mfiumara very good work

#251 (comment)

in many cases the engines have additional functionalities, so i think we have to export them as well

this fact is unfortunately not quite covered

try to explain it again
it can happen that an engine has additional methods beyond the basic functionality, which is totally ok
but currently they are not accessible, because you always return the general interface as return type of the "New" method
it's ok to return the original engine here, because internally it's ensured that it implements the methods of the interface and the users also have the possibility to pass the interface in their tests or methods

[ReneWerner87]

By the way, if you made the last change, do we have to count up the major because there is a breaking change included here ?

[mfiumara]

Thanks for the feedback @ReneWerner87. At the moment there are no extra methods being exposed by any of the engines as far as I can tell, which is why I'm returning the same interface for all the engines and have removed the interfaces defined in the template engines.

If you want I can define the interface separately again for each template engine and return that in the Newxxx functions, but none of them will be implementing any additional template specific functionality.

Regarding backwards compatibility, I'm not 100% sure how or when this package breaks backwards compatible as I'm not aware how external packages depend on this one. I can bump the version but not sure how you go about this in this project, maybe it's best if I leave that up to you to do once any other changes are agreed upon.