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

Add arbitrary expression parsing in curly braces … #46

Merged
merged 4 commits into from Jan 30, 2020
Merged

Add arbitrary expression parsing in curly braces … #46

merged 4 commits into from Jan 30, 2020

Conversation

brandonpittman
Copy link
Contributor

I found myself wanting to push the limits of what template can do, but I didn't want to pull in a larger templating library, so I figured we could just allow the template function to handle any expression inside it.

I found myself wanting to push the limits of what `template` can do, but I didn't want to pull in a larger templating library, so I figured we could just allow the template function to handle any expression inside it.
@selfrefactor
Copy link
Owner

Hi @brandonpittman
That sound like a smart idea.

I appreciate your PR, but I will have to ask you to add some tests, because as much as I see, there should be a change in R.template specs.

@selfrefactor
Copy link
Owner

Few more things:

  • VSCode complains about the naming of escape so maybe it should be something like escapeSpecialChars

  • You are missing export

  • Current test suite is failing with the following log:

FAIL src/template.spec.js

✓ within bracets (3ms)

✓ ok

✓ no interpolation + curry (1ms)

✕ with missing template input (4ms)

● with missing template input

ReferenceError: bar is not defined

at eval (eval at template (:22), <anonymous>:1:23)


@brandonpittman
Copy link
Contributor Author

I’ll work on that today. Thanks.

@brandonpittman
Copy link
Contributor Author

@selfrefactor I changed the escape name and exported the function.

I also changed the test because it shouldn't spit out the template string anymore. It's going to evaluate everything in the brackets, which should actually throw a reference error.

@selfrefactor
Copy link
Owner

I will check it later today. Thank you again.

@selfrefactor
Copy link
Owner

I copied your version of R.template and I ran it against the current Rambdax spec. This test:

test('with missing template input', () => {
  const input = 'foo is {{bar}} even {{a}} more'
  const templateInput = {
    baz : 'BAR',
    a   : 1,
  }

  const result = template(input, templateInput)
  const expectedResult = 'foo is {{bar}} even 1 more'

  expect(result).toEqual(expectedResult)
})

you know that fails. because in your tests you expect to throw. We would introduce a breaking change, this even forcing to major bump. Tests are contracts, and I think this contract of R.template not to throw on wrong input seems important.

Maybe you should change the method so it passes the failing test.

@brandonpittman
Copy link
Contributor Author

@selfrefactor Okay, we now just pass back the unprocessed input in the event of a ReferenceError. Existing specs pass and I added a new one for arbitrary JS expressions.

@selfrefactor
Copy link
Owner

selfrefactor commented Jan 30, 2020

@brandonpittman - that sounds good to me. As you may have understand, my main interest is not the code itself rather than what happens with the tests. I will wait for you to notify me once you are ready with the tests.

@brandonpittman
Copy link
Contributor Author

Not sure what you mean.

The latest commit makes all your tests pass and passes a new test I wrote to cover bare expressions.

@selfrefactor selfrefactor merged commit f38d2de into selfrefactor:master Jan 30, 2020
@selfrefactor
Copy link
Owner

selfrefactor commented Jan 30, 2020

I will release a version this Sunday. Thank you for your collaboration.

@brandonpittman
Copy link
Contributor Author

Thank you!

@selfrefactor
Copy link
Owner

Just released 3.6.0 including this MR.

@brandonpittman
Copy link
Contributor Author

@selfrefactor Still showing up at 3.5 on NPM.

@selfrefactor
Copy link
Owner

I realized that just now, and finally the release is made.

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.

None yet

2 participants