[Addressing] Country name to code converter extraction #4252
Conversation
lchrusciel
commented
Feb 22, 2016
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| License | MIT |
| * | ||
| * @return string | ||
| * | ||
| * @throws \InvalidArgumentException If name is not found in country code registry. |
There was a problem hiding this comment.
Should the @throws docblock be in interface? I think it's an implementation detail.
There was a problem hiding this comment.
IMHO the interface is right place for it, but I'd comment it with "If conversion fails"
There was a problem hiding this comment.
I think, that @Zales0123 is right. As far, as we don't depend on this exception it is an implementation detail. It should be placed in class.
There was a problem hiding this comment.
So what's the other way to tell the user that given country name cannot be transformed? It's one of the obvious cases that should be a part of a contract, otherwise there is no way to handle an transformation error.
There was a problem hiding this comment.
The interface know nothing about the exceptions since it's only abstraction layer. The exceptions are dependent on implementation and should be declared there.
There was a problem hiding this comment.
It knows nothing, but we can declare, that implementations will throw given type of exception if something bad happens, so the interface consumer can catch it and handle it. Throwing an exception is one of the ways to communicate between objects just like returning a value.
There was a problem hiding this comment.
I agree that while defining contract we can specify which exception should be thrown (it's a pity that PHP doesn't enforce it on implementations...). In that particular case it is totally valid to specify the exception 👍
|
👍 for extracting, after this is merged we can reuse it in contexts, fixtures and few other places in Sylius also :) |
08f4b7e to
59951a8
Compare
| @@ -17,6 +17,7 @@ | |||
| <import resource="services/pages.xml"/> | |||
| </imports> | |||
| <parameters> | |||
| <parameter key="sylius.behat.converter.country_name_to_code.class">Sylius\Behat\CountryNameToCodeConverter</parameter> | |||
There was a problem hiding this comment.
This should be removed
59951a8 to
476189b
Compare
| * | ||
| * @throws \InvalidArgumentException If name is not found in country code registry. | ||
| */ | ||
| public function convertToCode($name, $locale = 'en'); |
There was a problem hiding this comment.
We should not define defaults on interface.
|
It is ready to merge |
[Addressing] Country name to code converter extraction
|
Thanks Łukasz! 👍 |
…onverter [Addressing] Country name to code converter extraction