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 Support for IEnumerable tokens on TextTransformationExtensions #2124

Closed

Conversation

maldworth
Copy link

I would like to add the ability for adding a dictionary of tokens.

I have a JSON file of key values, and when I use newtonsoft to deserialize, it defaults into an IDictionary<string,object>, and so I'd like to simply pass that into the TextTransform Extension Method.

@dnfclas
Copy link

dnfclas commented Apr 11, 2018

CLA assistant check
All CLA requirements met.

@maldworth
Copy link
Author

Updated as requested in feedback.

@gep13
Copy link
Member

gep13 commented Jul 25, 2018

@maldworth quick question... Is there an open issue associated with this pull request?

@gep13 gep13 changed the title Add Support for IDictionary tokens GHXXXX: Add Support for IDictionary tokens Jul 25, 2018
@maldworth
Copy link
Author

@gep13 No open issue. I just thought this would be helpful to have this option

@patriksvensson I believe I made the changes as requested. Can you take another look when you have a chance?

thank you

@gitfool
Copy link
Contributor

gitfool commented Dec 7, 2018

@gep13 I recently added a similar request in #2375 before discovering this PR just now.

@maldworth @patriksvensson can we sort out the final details and merge this in time for Cake 0.31? Before that however, I do have a couple of suggestions for my own requirements.

As per the sample code in my issue, this extension method would be much more convenient and useful if:

  • do not throw if the tokens parameter is null
  • do not register tokens without a value

Re the second point, from experience I've found it much cleaner and safer to leave token placeholders behind for tokens without a value. This is especially helpful if staging a template after rendering, before publishing as an artifact, as it helps to diagnose issues at build-time.

Also, it would be nice if we could include my ToTokens extension for shallow object properties. 😉

@maldworth
Copy link
Author

@gitfool if it was silent to missing tokens or null params, then perhaps it would be a .TryWithTokens(). Or perhaps add a parameter to ignore missing tokens? anyways it would be nice to see this merged in sooner rather than later. Or we can combine both into one PR. Doesn't matter to me.

@gitfool
Copy link
Contributor

gitfool commented Dec 8, 2018

@maldworth your suggested behavior is problematic as TextTransformationTemplate.Replace already silently replaces tokens without a value, technically with a null value, with empty strings. These can be hard to spot, from recent experience, whereas leaving the token placeholders behind clearly stands out. Note that that would also be equivalent to not registering the token at all, which can happen, so has a nice consistency to it. (This is not a problem when calling TextTransformationTemplate.Render directly as you can conditionally call it.)

I suppose the other option would be to throw an exception if a token value is null, and ideally this behaviour is managed and enforced in one place, by TextTransformationTemplate itself, so you can register tokens with null values, either directly or via the proposed dictionary extension, and Render will later enforce its policy. This would require plumbing settings and may complicate the API unnecessarily, so let's wait for feedback from the Cake maintainers.

@patriksvensson patriksvensson self-assigned this Mar 21, 2019
@patriksvensson
Copy link
Member

This have been merged. Sorry for the delay...
Thank you for your contribution 👍

@gep13 gep13 changed the title GHXXXX: Add Support for IDictionary tokens Add Support for IDictionary tokens Mar 21, 2019
@gep13 gep13 added this to the v0.33.0 milestone Mar 21, 2019
@gep13 gep13 changed the title Add Support for IDictionary tokens Add Support for IEnumerable tokens on TextTransformationExtensions Mar 28, 2019
@gep13
Copy link
Member

gep13 commented Mar 28, 2019

As a result of this PR #2509 the overall aim of this PR has changed a little bit, so I have updated the title of this one (which will be included in the release notes) to reflect the final outcome.

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

Successfully merging this pull request may close these issues.

6 participants