phone-number: Regenerate Tests

[pgaspar]

Update phone-number tests to: 1.6.0 a317aa4

Update generator to use the standard error indicator.
Update solution to pass updated tests.

[Insti]

I'm not sure ArgumentError is the right thing to be doing here.

There's nothing really wrong with the argument, it's a string that we want to "clean".
So I think that nil was a reasonable Rubyish way to indicate an unclean string.

I'd be interested in hearing any arguments about why ArgumentError could be the right thing to do here though.

[pgaspar]

Hello! My bad, using ArgumentError was an oversight on my part.

As for wether this should raise an error or return nil, I'm not entirely sure. The reason why I made it raise an error was because of exercism/problem-specifications#1311 and specifically this quoted text:

If an error is expected (because the input is invalid, or any other reason), the value at "expected" should be an object containing exactly one property, "error", whose value is a string.

phone-number's canonical problem specification was recently changed (on exercism/problem-specifications#1337) to expect an error object instead of nil.

This is why my interpretation was that this should raise an error. If this should expect nil, shouldn't the canonical specification be changed to reflect that instead of doing something specific here?

This being said, which other exception do you think we should use here? I'm going to look at the other generators to get some ideas...

[pgaspar]

I had a look on the canonical data file for this problem and these were the comments associated with all the error cases:

incorrect number of digits
11 digits must start with 1
more than 11 digits
alphanumerics not permitted
punctuations not permitted
area code cannot start with zero
area code cannot start with one
exchange cannot start with zero
exchange cannot start with one
area code cannot start with zero
area code cannot start with one
exchange code cannot start with zero
exchange code cannot start with one

The main comment on that file also states:

Returns the cleaned phone number if given number is valid, else returns error object.

So maybe we could have a PhoneNumber::InvalidNumberError exception? I'm leaning to this inheriting from ArgumentError, after reading these things above ^

[Insti]

PhoneNumber::InvalidNumberError is a good idea.

Hypothetical question:

If you were going to use this method as part of a bigger application, how would you want to use it?

[pgaspar]

I think it depends. One use I can think of is running this as a pre-processing step to convert user-provided input to a format the system is able to work with. Imagining that data was submitted in a web form, I'd expect the form to show a validation error, pointing out the invalid input. Raising an exception seems more appropriate than returning nil in that scenario, I think.

Of course I can also come up with situations where returning nil would make sense..

Maybe what's a bit confusing is that the method now does both data validation and data cleanup, looking at the canonical data I linked above. If that makes sense or not is an interesting discussion, but maybe it makes sense to have it at the canonical data level, not specifically in the Ruby track - what do you think? I'm assuming that if the canonical data says it expects an error, we should raise an error - and that individual tracks are not encouraged to stray too much off the path and make their own decisions. But maybe that's not the case?

[Insti]

I really want to continue this discussion, but I'm a bit busy for the next few days. Give this thread a bump next week and I'll be able to give some better feedback ❤️

[pgaspar]

Very interesting, thank you for sharing those links! I see this is a hotly debated topic and I understand both arguments. I mostly agree with your point of view, specially in exercises that are focused on something other than data validation.

In this case, however, reading phone-number's description, there seems to be a lot of emphasis on the validation part of the exercise.. They're even present in the original readme. So maybe in this exercise it does make sense to test for those invalid cases, what do you think?

Anyway, I see at the bottom of exercism/problem-specifications#902 that you and others removed tests for invalid input and blank strings from some of the exercises. If you think that should also be the case in this exercise, I think the way to go would be to change the canonical data so changes are applied everywhere. While that doesn't change, I propose we go with PhoneNumber::InvalidNumberError 👍 (I'll likely push those changes tomorrow and I'll ping you in a week - we can rollback if needed).

[emcoding]

Interesting discussion!
However, I am not sold on raising an error in the first place. This exercise is about cleaning the input, and I consider the cleaning as a sort of (weird) validation. In this case, returning nil seems more informative than raising an Error.
I'd say raising an error is mostly useful for cases where the user is able to change something in the input, and that seems not to be the case here.

Apart from that, I like PhoneNumber as a possible candidate for a core exercise early in the track, and probably at a stage where I'd prefer to avoid that student create custom errors. (Because it can be solved with really basic Ruby, while being quite an interesting/challenging exercise for beginners.)

Because of both reasons, I'd vote for leaving the tests as is for now. And revisit in a few weeks, when we know more about the position of this exercise in the track. How would the two of you feel about that?

[Insti]

Ok, I've read through all this again, and agree with @F3PiX here.
I think we should use nil as the result of invalid phone numbers.

[pgaspar]

Ok, @Insti @F3PiX, I'm going to push those changes soon. I'm pushing a new commit, but let me know if I should amend the existing one instead 👍

[Insti]

Expect nil for the "error" cases.

[pgaspar]

@Insti changes are up for review 👍 Thank you for the discussion!

[Insti]

For reference the discussion is here: #870 (review) you might need to "view outdated"

[emcoding]

🎈