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 Fn::Split #141

Merged
merged 1 commit into from
Feb 16, 2017
Merged

Conversation

bpholt
Copy link
Contributor

@bpholt bpholt commented Feb 9, 2017

Resolves #140.

Because Fn::Split logically returns Seq[Token[String]], I wanted to make sure it could be used anywhere Seq[Token[String]] is used today. That's why I added the TokenSeq[R] (which is a type alias for an Either): either the caller can use a hard-coded sequence of strings (or Token[String]s), or they can use Fn::Split. In the first case, the resulting JSON is a JsArray, and in the second, the resulting JSON is a JsObject. Either way, the Either should be fairly transparent to the caller because there are implicit functions that lift values into the Left or Right as appropriate.

@bkrodgers
Copy link
Contributor

Thanks for the submission! So is your expectation that with the implicits you've added that this would be fully backwards compatible with existing templates? If so I'll test it out on a few of our existing templates to confirm.

Copy link
Contributor

@bkrodgers bkrodgers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I built this and tried it on one of our old templates. Seems to have worked fine with no changes to the template.

@tj-corrigan , could you give this a look too before I merge it?

@bpholt
Copy link
Contributor Author

bpholt commented Feb 10, 2017

Glad it works with your templates! It was my intention that the implicits would make it backwards-compatible. It seems to be so as far as I could tell, but we don't use all the AWS products this PR touched, so I couldn't be 100% sure.

@bkrodgers
Copy link
Contributor

TJ hasn't had time to look at this, but I looked at it again myself and I think we're good to go. Merging.

@bkrodgers bkrodgers merged commit ce1ceac into Bayer-Group:master Feb 16, 2017
@bpholt bpholt deleted the split-function branch February 16, 2017 20:51
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.

3 participants