search

byrokrat / id

Data types for swedish personal identity and corporation id numbers.
The Unlicense
14 stars 3 forks source link

Created test for factory decorator and added scalar type hints #20

Closed jongotlin closed 6 years ago

jongotlin commented 6 years ago

Issue #17

jongotlin commented 6 years ago

Thanks for your review! Updated with correct mocked interfaces added some comments.

hanneskod commented 6 years ago

Hi and again sorry for the hold up.. I see your point in not writing tests with only mocks. The way phpunits mocking model works I think the relevant lines in AbstractFactoryDecorator would still be tested, but it would in a way be an odd test anyway, I agree.

I don't think the createId argument should be nullable though. The interface consumes a string so that is how we should declare the type in my opinion. That also makes it possible to use statical analysis to find places where null is being passed. TypeError is good as passing null is an error much in that same way as calling a non-existing method or simliar.. I see no problem there.

jongotlin commented 6 years ago

Updated to allow null. Should we leave the test as is or do you want me to change something there?

hanneskod commented 6 years ago

Yupp, looks good to me. Thank you! Merging..