google-docs-mustaches icon indicating copy to clipboard operation
google-docs-mustaches copied to clipboard

:building_construction: Refactoring internal code

Open Errorname opened this issue 4 years ago ā€¢ 2 comments

This PR refactors the engine behind google-docs-mustaches, but keep the same API and almost the same behavior.

List of changes:

  • Improved placeholder parser (better quote handling)
  • Mustaches now work inside headers and footers
  • Fixed npm run token for local development
  • Upgrade dependencies
  • Fixes #25 concerning update generation not having side effects on each others

What's left to do before merging:

  • [Ā ] Write tests

Errorname avatar Oct 22 '20 23:10 Errorname

It looks really, really good and works fantastic. Alright, so here is my testing review so far:

I like how they if you don't provide a Placeholder the Placeholder will stay in the document and won't get replaced. This is not only good for my specific use case because I want to merge half of my placeholders in the first merge and the other half of the placeholders in the second step. But this behaviour is also good for debugging because you can way better spot still existing placeholders that tell you the merge didn't quite work, than you can spot empty strings that got parsed. But this doesn't work if your placeholder is used together with a formatter. Then the empty string is parsed again. Maybe I'll later have a look at it, on if and how this can also be prevented.

I noticed that this PR needs a little documentation update, since formatters are now high order functions if you pass first arguments directly from the placeholder. A placeholder with a custom formatter like so {{ value | someCustomeFormatter(1,2,"abc") }} now needs to have a custom formatter function with a signature like this someCustomFormatter = (number1, number2, string1) => (parsedPlaceholderValue) => ..... Before this PR it needed to look like this comeCustomFormatter = (parsedPlaceholderValue, number1, number2, string1) => ....

And my last point is rather a little feature addition that I'd like to propose although it's also something that worked with the current library version and would break with this PR. I'm talking about the ability to have multi word Placeholder variables like this one here {{ Some Placeholder }}. This two word placeholder worked before but currently breaks because your parsers is very sensitive about spaces and wants to create a new token out of the second word. I already went ahead and adjusted the Regex in such a way that multi word placeholders work again. I'm going to put up a PR to merge my changes into this branch right here and then you can test it and give me feedback about it.

Regarding #46 , I haven't put any work in so far, because I wanted to test this version first and I was quite surprised to see that the Placeholders now keep their exact casing all the way through the merging Process, so my initial issue in #46 being that all placeholders are lower cased is kind of resolved. But again, having not to worry about casing at all is definitely a really good feature for this library and I try to put up a PR for that as soon as possible

8BitJonny avatar Oct 31 '20 12:10 8BitJonny

Here are my two my proposals for:

  • fixing the Multi Word Placeholders: #49
  • fixing not replace behaviour for not provided placeholders even with applied formatters: #50

8BitJonny avatar Oct 31 '20 15:10 8BitJonny