splitting leaves objects within split content #73

Closed
taf2 opened this Issue Feb 11, 2013 · 8 comments

Comments

Projects
None yet
2 participants

taf2 commented Feb 11, 2013

Example:

Phony.split(Phony.normalize("+972"))

->

["972", "[#<Phony::NationalCode:0x007ffafe4d9fa8 @national_splitter=#<Phony::NationalSplitters::None:0x007ffafe4be190>, @local_splitter=#<Phony::NationalSplitters::Default:0x007ffafe4c30c8>, @normalize=true>]"]

expected:

["972",nil,nil,nil]

taf2 commented Feb 11, 2013

obviously not ideal, but an immediate work around is: https://gist.github.com/taf2/4754606

taf2 commented Feb 11, 2013

Also effected is calls to format

Owner

floere commented Feb 11, 2013

Hi Todd,

I like that you take action to fix the problem for yourself. I have commented on the gist.

Format uses split, which is why it is also affected.

A quick question, first – why do you expect 3 nils? (Just making sure I understand)

Cheers and thanks!

taf2 commented Feb 12, 2013

Ah sorry, i had that incorrect in the gist - moving too fast this morning. I had actually just 1 nil - which i bet was why you're asking...

Owner

floere commented Feb 12, 2013

Thanks Todd!

Exactly, your guess is on the money :)

I'm interested as to when this case would occur. I am trying to be a bit resistant to changes which open up Phony to non E164 conformant numbers, as I designed it such that only Strings which contain correct E164 numbers can be used.

One of the reasonings is: I'd like to remind people that not the view should be responsible for formatting all kinds of data, but to handle this before, e.g. when saving the data in a persistent data store, to only allow correct E164 numbers in there, which then enables any kind of formatting of numbers (not just with Phony, but other libs which might render E164 numbers in some kind of way). If your phone numbers don't adhere to some standard in the e.g. database then the rendering libraries all need to handle the shortcomings.

I hope I am making sense, and probably I am a bit academic on this one. I am mainly wondering in general, but also in your case whether you're looking for the problem in the right place. If you have thoughts on this, that would be interesting to hear.

Cheers

taf2 commented Feb 12, 2013

I agree, this should be a rare case and generally something that the application should handle, my thoughts here though are that Phony objects should probably never be serialized to string regardless of input... Also, perhaps in a lesser degree there is something Phony.normalize("+972") could do to transform the input - but unlikely because I agree with your point that E164 numbers should be expected. I think the main issue here is in serialization of Phony objects in an output string... So, in this case either a nil should be returned or perhaps a format exception raised?

Owner

floere commented Feb 21, 2013

Sorry for the late feedback – I agree on the main issue. I'll have a look on what we could do to return nils.

@floere floere closed this in 8e113a0 Feb 21, 2013

Owner

floere commented Feb 21, 2013

See the above commit :) I've just released 1.9.0, which fixes the problem with #normalize returning Phony internals. Thanks for your input and help!

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