[PATCH] Query::Conditions::Comparison documentation, specs, and some fixes
(Really hope I'm not stepping on anyone's toes with these patches...)
I'd like to submit for your consideration four patches concerning the Query::Conditions::Comparison classes in DataMapper 0.10 (dm-core/next). I've committed each to my own dm-core repo on Github, so you can pull from that if you find it easier than applying the attached patches.
Patch: 0001 /
Adds fairly extensive yardoc documentation about the various Comparison classes.
Patch: 0002 /
Adds specification for AbstractComparison#valid? (and subclasses) using a shared example.
Patch: 0003 /
Adds specs for relationships when used in EqualToComparison and InclusionComparison.
Patch: 0004 /
This patch is what I originally set out to do; as you can see I went a bit mad with the previous three. :)
After installing DM 0.10 from Github, I found that some of my relationships which use custom types (Enum) weren't working as they were in 0.9. I eventually tracked it down to the fact that the Comparison class stored values as they they are in the repository (dumped), rather than storing the custom, loaded value. The dumped value was then passed to Property#valid? which attempted to dump it again, resulting in it returning false.
This patch adds a "loaded_value" attribute to the Comparison class which is used with Property#valid?, and when doing filtering with #matches?
Comments and changes to this ticket
- Assigned user set to Dan Kubb (dkubb)
- State changed from new to accepted
@Anthony: These are fantastic. Thanks for going the extra mile and including specs and docs with your patches. Give me a bit of time to review these and add any comments, but by my preliminary review they look really good.
I noticed you included the docs using YARD format. Were you working with any guidelines, or are these something you use on personal projects? The reason I ask is I am developing a set of guidelines for YARD documentation usage in DataMapper and baking them into a tool I am developing: http://github.com/dkubb/yardstick
I noticed you included the docs using YARD format. Were you working with any guidelines, or are these something you use on personal projects?
I don't use a specific set of guidelines, but I always document arguments, returns values (except where a method doesn't return anything of importance), and use @yield, @yieldparams, and @yieldreturn where applicable. I tend to use @example where doing so provides extra clarity about what a method or class does.
I took a look ay Yardstick and the rules seem perfectly reasonable to me, the only exception being "The method summary should not end in a period". I suppose it's a matter of personal taste, but I'd probably break that rule in my own projects; sentences look... incomplete without one. :)
I've attached a fifth patch to the ticket (0005 / http://github.com/antw/dm-core/commit/9040909e2f64014f6bdd895b31844...) which reduces the number of errors Yardstick raises on my Comparison docs (from 111 to 23). A few things I noticed...
- Yardstick wanted @example to be present in documentation for attr_*, which seems a little redundant. Similarly, I'm not sure if there is much value in providing examples for methods which don't accept any arguments (e.g. AbstractComparison#valid?/#relationship?/#property?/#hash, etc), while others like #== are fairly self-explanatory — I think well-written specs reduce the need for examples on every method.
- It also wanted @return to be present on #initialize, which seems fairly unnecessary.
Those (minor) critiques aside, I'll certainly look into using Yardstick on my own projects.
Yeah, it's possible I went a little overboard with the "summary should not end in a period". That's a personal convention I baked in, but I should probably remove it.
I agree with not requiring @example for methods that don't accept any arguments. Those @example sections end up looking useless since there's not really anything to describe.
Perhaps I should loosen the @example requirement across the board, and let the developer decide when it is appropriate or not.
You're right about #initialize requiring @return being wrong. It doesn't matter what you return from that method anyway, the new constructor calls #initialize on the instance, and then returns the instance no matter what you return from #initialize.
I plan to extend Yardstick to ensure that when @yield is defined, it matches @yieldparam, and that @yieldreturn is specified too. If you have other ideas for more rules in Yardstick please let me know.
- State changed from accepted to resolved
(from [70e3c902849de001dfc872bfd095274ce3c89552]) Minor formatting changes to antw's excellent documentation patches