Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏗️ Refactoring internal code #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

🏗️ Refactoring internal code #48

wants to merge 1 commit into from

Conversation

Errorname
Copy link
Owner

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 Better updates generation #25 concerning update generation not having side effects on each others

What's left to do before merging:

  • Write tests

@8BitJonny
Copy link
Contributor

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
Copy link
Contributor

Here are my two my proposals for:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better updates generation
2 participants