#997 ✓resolved
Anthony Williams

[PATCH] Query::Conditions::Comparison documentation, specs, and some fixes

Reported by Anthony Williams | August 1st, 2009 @ 04:45 PM | in 0.10.0

(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 / http://github.com/antw/dm-core/commit/810ee85d3e3198a8d35c454bbc3d4...
Adds fairly extensive yardoc documentation about the various Comparison classes.

Patch: 0002 / http://github.com/antw/dm-core/commit/d74caffeed9c7646fc7a4d18a149c...
Adds specification for AbstractComparison#valid? (and subclasses) using a shared example.

Patch: 0003 / http://github.com/antw/dm-core/commit/d01c3497cfc2e9320666b1f699e6c...
Adds specs for relationships when used in EqualToComparison and InclusionComparison.

Patch: 0004 / http://github.com/antw/dm-core/commit/93c095104742aad25fbdd5a6370a3...
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

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) August 1st, 2009 @ 07:40 PM

    • 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

  • Anthony Williams

    Anthony Williams August 3rd, 2009 @ 08:36 AM

    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.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) August 12th, 2009 @ 11:38 AM

    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.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) August 12th, 2009 @ 11:39 AM

    • Milestone set to 0.10.0
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) August 13th, 2009 @ 11:26 PM

    • State changed from “accepted” to “resolved”

    (from [70e3c902849de001dfc872bfd095274ce3c89552]) Minor formatting changes to antw's excellent documentation patches

    [#997 state:resolved] http://github.com/datamapper/dm-core/commit/70e3c902849de001dfc872b...

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Referenced by

Pages