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

Phony.normalize should return a custom exception object when it can't parse a value #76

Closed
ajsharp opened this issue Mar 9, 2013 · 3 comments

Comments

@ajsharp
Copy link

ajsharp commented Mar 9, 2013

If you pass normalize a value it can't process, the following exception gets raised:

Phony.normalize('!') # => <NoMethodError: undefined method `normalize' for 1:Fixnum>

It seems like Phony should raise a more useful exception here that downstream code can reasonably deal with:

def normalize_phone_number(num)
  Phony.normalize(num)
rescue Phony::ParseError => ex
  errors.add(:base, "#{num} is not a valid number")
end

Happy to write a patch if you agree with this approach.

@floere
Copy link
Owner

floere commented Mar 13, 2013

Hi @ajsharp, thanks for the nice issue entry.

Can you reason with me why your approach is superior to:

def normalize_phone_number(num)
  Phony.normalize(num)
rescue
  errors.add(:base, "#{num} is not a valid number")
end

Your approach enables people to also rescue from other types of exceptions. There are two cases where this is useful:

  • Calling a given method (ie. Phony.normalize) can raise multiple types of exception, and discerning them is important. This is not the case. Phony simply fails if there is not enough information, and cannot discern why easily.
  • More code than Phony.normalize(num) is called, which may raise other types of exceptions. Personally, I'd like to discourage people from bunching code up that can throw multiple exceptions.

What are your thoughts?

@floere
Copy link
Owner

floere commented Mar 13, 2013

Also see https://github.com/floere/phony#normalizing: "This will often raise an error if you try normalizing a non E164-izable number (a number that does not contain enough information to be normalized into an E164 conform number). Use Phony.plausible? for checking if it can be normalized first."

But please disagree – I really welcome well-reasoned disagreement :)

@floere
Copy link
Owner

floere commented Apr 29, 2013

See #87.

@floere floere closed this as completed Apr 29, 2013
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

No branches or pull requests

2 participants