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

Formats trait values to trim spaces instead of replacing them with underscore #19

Conversation

cem2ran
Copy link
Contributor

@cem2ran cem2ran commented Jan 24, 2018

I found it quite odd behaviour to format values such that spaces are replaced with underscore.
If I were to send the name of the user or similar I would have the following as the value: 'Cem_Turan' instead of 'Cem Turan'.

This PR changes the behaviour such that surrounding whitespaces and new lines are trimmed.

@ladanazita
Copy link
Contributor

ladanazita commented Jan 30, 2018

Hi @cem2ran ,

When building this, we found Firebase referenced using underscores in their documentation. It seems consistently used throughout their documentation, for example here.

Did you find evidence that this isn't the case?

Thanks!
Ladan

@cem2ran
Copy link
Contributor Author

cem2ran commented Jan 30, 2018

@ladanazita As far as I can tell there's only evidence for the trait name being underscored. Not the actual value. All values are referenced as variables, which you must presume has whatever format you feel is right.

@ladanazita
Copy link
Contributor

Make sense, can you also update the tests @cem2ran to coincide with the changes? Thanks!

@cem2ran
Copy link
Contributor Author

cem2ran commented Jan 30, 2018

Sure thing.

@ladanazita
Copy link
Contributor

Hi @cem2ran I'm sorry for the delay here. I will get this out this week!

@ladanazita ladanazita merged commit d3c642b into segment-integrations:master Feb 27, 2018
@ladanazita
Copy link
Contributor

ladanazita commented Feb 27, 2018

@cem2ran This will be live on version 2.1.0. Thanks for the contribution and your patience here!

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.

2 participants