twolfson / reverse-mustache

Reverse templating library for mustache, generating variables from a template's output
The Unlicense
30 stars 6 forks source link

Trim non-common suffixes with variable reusing #5

Closed adrianheine closed 7 years ago

adrianheine commented 7 years ago

Test says it all, I think.

twolfson commented 7 years ago

It's been a really long time since I have touched this repo. Can you provide more explanation as to the behavior before and new behavior?

adrianheine commented 7 years ago

Sure! Imagine you reuse a variable but have some stuff after it that could be part of the value:

{{variable1}} {{variable2}}
{{variable1}} {{variable3}}
Value1 CouldBe1Or2 Definitely2
Value1 Value3

It currently throws an error (at least for me) instead of backtracking.

twolfson commented 7 years ago

Ah, I see. That's quite annoying. I'm currently hesitant to land this as using try/catch seems unlike me. I think the assert there was more as a sanity check for the developer. Can you update it so we do a string comparison check + return instead?

twolfson commented 7 years ago

I will look into the failing Travis CI issues so we can get this fully green

twolfson commented 7 years ago

Alright, Travis CI is now up and running; we had to drop Node.js@0.8 and I figured to move to Node>=4 in the process as it's been out for a year and Node@0.10 is no longer maintained.

https://nodejs.org/en/blog/release/v4.0.0/

https://github.com/nodejs/LTS/tree/7ab8b0751b568e4af937493a9b94863d00a26be1

adrianheine commented 7 years ago

I'd extract the non-asserting part into a new method and replace addStr with a call to that method and return value assertion. That way we don't have to touch other callers. Sounds ok?

adrianheine commented 7 years ago

Rebased and introduced tryAddStr.

twolfson commented 7 years ago

The tryAddStr behavior feels weird since it could or could not add. Additionally addStr won't throw if it didn't work out. Maybe we could add a canAddStr method which returns true/false if it'll work out and keep addStr as is?

adrianheine commented 7 years ago

Hm, I think »try« pretty much conveys the meaning that it could or could not work. Why do you think addStr won't throw if it didn't work out? There is an assert that always fails. I'll have a stab at canAddStr, though.

twolfson commented 7 years ago

The previous setup had the assert in an if which made it not always run =/ The new setup looks good to me :+1: Going to land this and add you as a contributor (but please submit PRs still)

twolfson commented 7 years ago

This has been merged/released in 1.8.0

adrianheine commented 7 years ago

Thanks!