Refactor Ssn to implement IsoCodeInterface #73
Conversation
|
Well, indeed it's even more simple. But I think it's BC break because user won't be able to instantiate |
|
@soullivaneuh Nope, there is no warning. I believe it's not a BC break. https://3v4l.org/sFWY3 EDIT: Of course it is be BC break for those who were extending the Ssn class but that's probably rare. |
|
We might want to consider using |
Maybe add a BC test for this would be great. |
|
Done. There is something wrong with the Ssn tests though. They fail on my computer but it's not related to my patch. Thay fail on the current master branch as well. I'm NOT going to debug that. |
|
The author should add .travis.yml and setup running tests by travis to prevent that in the future. |
|
I don't understand. Travis is set up and is actually green here: https://travis-ci.org/ronanguilloux/IsoCodes/builds/83485286 |
|
Oh you mean deprecations error. Yeah, I submitted PR #57 for Travis improve. I'll propose something better to manage deprecations after that. 👍 |
|
No I don't mean deprecation errors. I mean actual asserts in SsnTest and CreditCardTest. Maybe it's windows-specific or something. I'm sorry I missed the travis somehow. |
|
|
|
Hmm, that's strange. Unfortunately, we can't reproduce it on Travis, Windows is not available. |
|
As for SsnTest the errors disappear if I switch the Ssn.php file from CRLF to LF. That should be considered a bug though... Dunno what's the problem with CreditCard. |
Could you please open a separate issue with more details about this? |
|
@soullivaneuh Anyway since the failing tests are not related let's get back to the topic. Do you consider this solution to be better than #34? If not, why? In my opinion we should decide which is better and close the other one. |
|
Your solution is more simple and cleaner IMO. But, even if This is my objective opinion. I let @ronanguilloux get the last decision. :-) |
|
Well adding a deprecation warning to __construct is trivial. ;-) |
Indeed. Let's do it so! :-) |
enumag
force-pushed
the
patch-1
branch
3 times, most recently
from
1c4cc1c
to
514bb28
Compare
|
@soullivaneuh Done. |
|
|
||
| public function __construct() | ||
| { | ||
| trigger_error('Instantiating Ssn validator is deprecated since version 1.3. Use Ssn::validate($ssn) instead.', E_USER_DEPRECATED); |
There was a problem hiding this comment.
- Deprecated error should be silenced (
@). - Use FQN for
Ssnclass name
There was a problem hiding this comment.
I was using the existing deprecated errors for reference - https://github.com/ronanguilloux/IsoCodes/blob/master/src/IsoCodes/Isbn10.php. There is no @ and FQN is not used. You can implement such changes in separate PR if you wish.
There was a problem hiding this comment.
Yeah. This was made before phpunit-bridge requires deprecated errors to be silenced.
I'll do a PR but please correct it on this one.
Ok for non-FQN.
There was a problem hiding this comment.
You mean to add a @ to the test right? Putting it here would suppress the warning for users.
I've read the recommendation in phpunit-bridge readme. I understand now.
|
Coverage decreased. Could you please take a look? |
|
I think that's because of the non-silenced deprecated errors. There is no way for this commit to really decrease code coverage. |
|
@ronanguilloux https://coveralls.io/builds/3761491/source?filename=IsoCodes%2FSsn.php
Could you fix that? Just have to put |
Refactor Ssn to implement IsoCodeInterface
Alternative to #34.