search

elseym / wo.istes.jetzt

3 stars 1 forks source link

fix time parsing in request path #10

Closed m3co-code closed 2 years ago

m3co-code commented 7 years ago

I don't really like making the time parsing as a public method but usually you don't test against internal functions. Thats why I made it public for now, knowing that maybe a broader refactoring should be done. Lets talk about it :)

m3co-code commented 7 years ago

Also I am up for more refactoring of the function, now that it is controlled under the test. Let me know whether you agree with the test behaviour :)

elseym commented 7 years ago

thanks for the test! i certainly should add more of them... ☘️ but i don't really like the idea of exporting utility functions just for testing. i'd rather go with a solution that puts the tests next to the implementation in their respective package. i'd then invoke the tests by leveraging go's ... wildcard (i.e. go test ./...). also, in order to resolve the bug addressed, i'd like to keep an implementation that juggles with ints rather than slicing strings and calling a parsing function multiple times (see my first comment in #9). how about i cook up a solution including some of your changes and request you pull it into your feature branch?

m3co-code commented 7 years ago

I was able to see how other golang projects are handling this and in fact the common pattern is to have them in the same package. This way you don't have to export the utility function. Regarding your suggestion to improve the implementation as in #9, SGTM :)