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

Show undefined value as empty string #19

Closed
jpiquot opened this issue Jun 1, 2021 · 23 comments · Fixed by #21
Closed

Show undefined value as empty string #19

jpiquot opened this issue Jun 1, 2021 · 23 comments · Fixed by #21

Comments

@jpiquot
Copy link

jpiquot commented Jun 1, 2021

I am using Pupa in my DevOps extension ChildTaskTemplate.
A user reported to me that undefined values are interpolated with the 'undefined' string Issue 5. Is it possible to configure Pupa to use empty strings instead of 'undefined'?

@sindresorhus
Copy link
Owner

Yeah, there could be an option to use empty strings on undefined.

@jpiquot jpiquot changed the title Show unkown value as empty string Show undefined value as empty string Jun 1, 2021
@yuhr
Copy link
Contributor

yuhr commented Jun 6, 2021

It woud be better if it can notify users when undefined, by throwing an error instead of embedding empty strings silently. Can I file a PR?

@jpiquot
Copy link
Author

jpiquot commented Jun 7, 2021

I think that this behavior should be defined by setup as there are plenty of business cases where undefined values should not throw an error but just silently put an empty string.

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

I see. Then the possible API would look like below:

const pupa = require('pupa')({
  emptyOnUndefined: true, // Default: `false`.
  throwOnUndefined: true, // Default: `false`. If `true`, it overrides `emptyOnUndefined` by its nature.
})

Or:

const pupa = require('pupa')({
  onInterpolate: (value, key, index) => value ?? "",
})

(I'd go for the onInterpolate way because it's generic and also covers multiple scenarios other than undefined)

Probably there might be some (edge-)cases to override some options at each templating, but it may go beyond pupa's original statement "Simple micro templating", so I feel a bit hesitate to add such API, which would be like this:

pupa(format, data, { ...someOverrideOptions })

What do you think about this idea?

@jpiquot
Copy link
Author

jpiquot commented Jun 7, 2021

Yes, the interpolate solution would be perfect.

@sindresorhus
Copy link
Owner

I think we should change the default behavior to throw on missing values. The common case is to have all the values, so should be loud by default in case of unexpected values.

Then we could introduce an option:

{
	missingValueBehavior: 'throw' | 'empty-string' | 'no-interpolate'
}

I'm happy to bikeshed the option name and the values.

I don't want to add onInterpolate as it seems we can solve this with a single easier option.

@sindresorhus
Copy link
Owner

I'm not 100% sold on falling back to empty string though. Can someone provide a short example of where falling back to an empty string makes sense? In such cases, I would argue, you should do the fallback in the code itself before passing the data to the template.

@jpiquot
Copy link
Author

jpiquot commented Jun 7, 2021

For example, you want to create child tasks from a DevOps workitem. You don't know anything about the custom fields created by the organization. How do you know what fields are used by the template? How can you dynamically add these empty fields to your data object if it's dynamic?

@sindresorhus
Copy link
Owner

I think I found a simpler way to handle this. Throw by default. And ability to not interpolate missing values:

{
	ignoreMissing: Boolean
}

And for the empty string case, we can do (as suggested earlier, with some small tweaks):

{
	transform: ({value}) => value ?? ''
}

Thoughts?

@jpiquot
Copy link
Author

jpiquot commented Jun 7, 2021

Yes thats nice.

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

{
	transform: ({value}) => value ?? ''
}

If the placeholder text is also passed to the function, then it becomes capable of performing "ignore missing" as well, by returning the placeholder as is, for example:

{
  transform: ({value, placeholder}) => value ?? placeholder
  // where placeholder is a string including braces i.e. "{{key}}"
}

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

And, "throw by default" behavior can be achieved by just having the default transform function be:

{
  transform: ({ value }) => {
    if (value !== undefined) return value
    else throw new Error()
  }
}

@sindresorhus
Copy link
Owner

If the placeholder text is also passed to the function, then it becomes capable of performing "ignore missing" as well, by returning the placeholder as is, for example:

True. But I still think it makes sense to have an option for this.

And, "throw by default" behavior can be achieved by just having the default transform function be:

No. I think throw by default should work no matter what. It would help catch problems if you accidentally return undefined in transform.

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

But I still think it makes sense to have an option for this.

Wait, just to clarify, do you mean that you don't want to pass the placeholder text to transform?

I understand having a dedicated option to do some idiomatic stuff makes the code more readable when you just want it , but I'd always love to go with "single logic to all purpose", and I would expect transform to be an escape hatch to perform arbitrary operation, which needs some additional data to be available. That includes:

  • value — the value itself, of course
  • key — the placeholder without braces
  • placeholder — with braces
  • index — how many placeholders occurred before the current one, in the format text

Though, if you feel this is not your design goal, just ignore me.

It would help catch problems if you accidentally return undefined in transform.

Hm, that's reasonable. Thanks pointing out.

@sindresorhus
Copy link
Owner

Wait, just to clarify, do you mean that you don't want to pass the placeholder text to transform?

I'm not against passing the placeholder text to transform.

and I would expect transform to be an escape hatch to perform arbitrary operation

I agree.

placeholder — with braces

Why does it need to include the braces?

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

Why does it need to include the braces?

Because pupa's braces are single or double, and users (i.e. writers of this function) can't tell which one the original placeholder was. That is, without this info, transform can't conditionally fallback to "ignore" behavior correctly.

@sindresorhus
Copy link
Owner

But Pupa handles escaping, so transform doesn't need to care about that. Whether it's {} or {{}} shouldn't matter.

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

Then how do you achieve "ignore if parseInt(value) is NaN, otherwise wrap it with angle brackets" for example? I thought I could write it like this:

transform: ({ value, placeholder }) => !Number.isNaN(parseInt(value)) ? `<${value}>` : placeholder

And I get:

pupa("{{0}} {{1}}", ["42", "undefined"]) // => "&lt;42&gt; {{1}}"

But if original braces wasn't provided, this is not possible, isn't it?

@yuhr
Copy link
Contributor

yuhr commented Jun 7, 2021

Okay I got it, you mean I have to use ignoreMissing option in such cases like this, right?

ignoreMissing: true,
transform: ({ value }) => !Number.isNaN(parseInt(value)) ? `<${value}>` : undefined

Hm, it feels a bit weird solution for me though, I can still throw manually if needed, so it's okay...

@sindresorhus
Copy link
Owner

you mean I have to use ignoreMissing option in such cases like this, right?

Yes

@sindresorhus
Copy link
Owner

This is now "pull request" welcome.

@yuhr
Copy link
Contributor

yuhr commented Jun 8, 2021

How about what timing it should accept options? Come to think of it, const pupa = require("pupa")({ ...options })-style API will introduce an unnecessary breaking change, so I'd suggest rather accepting at each templating, and if one wants to bake options in, then they can wrap it like:

const imago = (template, data) => pupa(template, data, { ...options })

@sindresorhus
Copy link
Owner

so I'd suggest rather accepting at each templating

Yes, that was the plan all along. I missed that your previous example did not do this.

It should be pupa(template, data, options?)

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

Successfully merging a pull request may close this issue.

3 participants