Closed sfinnie closed 2 years ago
Totals | |
---|---|
Change from base Build 1410940845: | 0.0% |
Covered Lines: | 2318 |
Relevant Lines: | 2318 |
Thanks! Well spotted!
Would be good to actually exercise this in the example, using a date?
Also perhaps making it possible to pass in the class, and have the application construct the instance, or maybe just define a list of classes on the application class, and have the base class construct and register all of them?
Two further PRs, perhaps :))
Would be good to actually exercise this in the example, using a date?
Yes, think that would be good. Always helpful to see example usage. I'll have a look at that when I get a chance, maybe this eve.
Also perhaps making it possible to pass in the class, and have the application construct the instance, or maybe just define a list of classes on the application class, and have the base class construct and register all of them?
I think passing an instance is fine; not sure passing the class makes things easier/more intuitive for the user (at least as far as I can see). Passing a list instead of multiple calls to register()
would be a little more convenient perhaps, but again possibly at the risk of increased complexity if there's only one. However, making multiple register()
calls doesn't seem overbearing or inconvenient. If the example was extended to include exercising the transcoding (per your suggestion above) I don't expect anyone would have problems or concerns.
Think the example is missing parentheses on the end of
DateAsISO
. As it stands, the code is passing the Class; whereasTranscoder.register()
takes an instance. Haven't tried with the specific example code but have erified with a different custom transcoding.