Enhancements to exceptions and BE, LU formatters #13
Conversation
|
@BenMorel did you get a chance to look at this? Let me know what you think, i could really use this in the package. |
|
Hi @rubentebogt, sorry for not getting back to you earlier! I'm generally 👍 on this pull request, I'll get back to you soon with some feedback (probably over the weekend). |
|
@BenMorel great! That would be awesome, i fixed some conflicts after your latest changes/release. Maybe with one or two commits to many so maybe you should squash when eventually merging. Looking forward to your feedback. |
tests/PostcodeFormatterTest.php
Outdated
| ['PL', '12345', '12-345'], | ||
| ['AT', 'A-8084', '8084'], | ||
| ['BE', 'B-1245', '1245'], | ||
| ['LU', 'L2556', '2556'], |
There was a problem hiding this comment.
Could you please move these country-specific tests to their respective test classes?
The tests here as only there to ensure that the code leverages the appropriate country formatter and are purposely kept small.
Please also add tests in each country formatter, to ensure that another letter used as prefix returns null.
For example, in ATFormatterTest:
['A-8084', '8084'],
['B-8084', null],
There was a problem hiding this comment.
@BenMorel I just did that but there is some inconsistent behaviour in the tests, this happens because for the PostcodeFormatterTest.php file the global PostcodeFormatter class is used but for the individual tests it's not. This results in input not automatically being uppercased and also spaces not being removed etc. as happens for the other tests and also for the public API.
In my opinion the abstract class CountryPostcodeFormatterTest should use the global PostcodeFormatter and then get the CountryCode via a constant or (there it is again :-) Reflection or something. This wil also test and ensure that classes are named properly and autoloaded correctly.
There was a problem hiding this comment.
Yeah sorry, I meant without the dashes:
['A8084', '8084'],
['B8084', null],
In my opinion the abstract class CountryPostcodeFormatterTest should use the global PostcodeFormatter and then get the CountryCode via a constant or (there it is again :-) Reflection or something.
I understand where you're coming from. The idea is to test each country formatter in isolation (a unit test), without repeating the tests for "strip dashes and spaces" etc., which is the responsibility of the PostcodeFormatter tests.
This wil also test and ensure that classes are named properly and autoloaded correctly.
That's a valid point. To improve this, we could:
- either add another test to each
CountryPostcodeFormatter, that invokes thePostcodeFormatter - or (preferred, IMO), ensure that each supported country has a test in
PostcodeFormatterTest(I'm not sure if more than one test is needed here—we don't want to repeat the unit tests—but typically the one test would be dash- or space-separated as appropriate for the country)
What about discussing this in another PR? We already have 2 unrelated changes in the present one.
There was a problem hiding this comment.
Yes i get the idea, ofcourse unit tests are ment to test units, the only problem is that the stripe here is part of the format i'm trying to test and implement. And so i want it to be in the test somewhere. And also test that others are or different cases are still failing.
Becasue what if you are changing the main class PostcodeFormatter, and remove the stripping of the stripes or spaces or add some other modification, then there is no way to test if this particular format still works. Or what if the removing of stripes is conflicting with one of the other formats.
But you are right let's indeed open a separate PR for this. For now i made it work with the latest commit.
|
@BenMorel Hi Ben, i changed a lot and reviewed your comments and made some of my own, basically now it comes down to the comment about testing: #13 (comment). Because if i remove all stripes and spaces all the tests work. Thanks for taking a look :-) |
|
@BenMorel Hi ben, in light of your recent comment i indeed agree for a different pull request on testing. Thus i removed the stripes and casing tests and now all build passes :-) |
|
@BenMorel Hi Ben, did you get a chance to look at this? I could really use this being released, and if so i will start to work on the testing PR like we discussed after that. |
|
Thank you, @rubentebogt! Sorry for the delay. |
|
Released as 0.4.0. |
* Enhancements to exceptions and BE, LU formatters (brick#13) * Make classes final --------- Co-authored-by: Benjamin Morel <[email protected]>
Adds getters to exception objects for easier use, for instance for displaying translated error messages to the user on a higher level, then it's nice you can grab just the postcode instead of the total exception message.
Implements a helper trait to strip off Country code prefixes from postalcodes:
In some countries like Belgium or Luxembourg its common to provide a country code prefix in a postalcode, although the format is incorrect, many government agencies, businesses and people still use this. See: https://en.wikipedia.org/wiki/Postal_codes_in_Belgium Rather than failing this format into an exception, this helper class strips the prefix off before validating it, thus providing you with the correct format without the country code prefix in it.
Of course a helper is not necessarily needed, but this prevents duplicate lines of code in the project and allows for other helpers to be shared across formatters.
Let me know what you guys think.