[DBIx-Class-Devel] Pull Request: topic/docs-pod-inherit

[Brendan Byrd]

On Mon, Dec 3, 2012 at 9:42 PM, Peter Rabbitson <rabbit+dbic at rabbit.us> wrote:
>
> So - review time. This generally is excellent work. There are a number
> of things I noticed - all listed below. Note - I have an OCD-ish eye,
> this is not an exercise to make your life difficult. The first section
> are the important issues, the rest are for your consideration.

That's fine, though my reply will be equally as detailed :)

> === Problems:
>
> = 03bf20842
>
> The wholesale .gitignoring of *.pod worries me. Please go back to
> listing individual autogenerated files. After all there are just 4 of
> them if I counted right.

Nono, there's a POD for each module that is a subclass of something else.
Somewhere around 20-30.  Hence I did it as inclusive and not exclusive in
the .gitignore file.

> The IC::DT documentation was modified to explicitly no longer mention
> DateTime::Format::DBI in 1b23a127. Please revisit the conflict resolution.

I thought that was a weird merge when I refreshed master.  I'll
restore that file
back to the original.

> (since I wrote this I saw ce2fdc6b addresses it)
> Almost all docs dead-link to L<DBIx::Class::ResultClass>. Did you lose it
> during rebase?

This was a decision to enhance commit readability, since all of those little
changes would bury the rest.

> In Relationship you used an L<> in a literal block - neither s.c.o. nor
> metacpan will render this properly (it will be left verbatim, since it's
> a literal block).

Whoops; overzealous in the replacement.  Will correct.

> In Relationship::Base::search_related the docs are wrong - one never could
> call it on a result source.

Not mine, but will correct.

> In Relationship::Base::count_related you added a thing about result
> source callability - it was not there before.

I might have been duplicating the erroneous info from search_related.  I'm
beginning to think that we should just remove that last sentence from both
of them and let the Inherited Methods sections sort it out.

> In Relationship::Base you use "undefined" as Return Value in several
> places. This could be confused with undef() - I would use "not defined".

I actually agree, but Manual::Reading stated otherwise.  I would be in favor
of a global change in that regard.

> In ResultSet::as_query and ResultSet::Column::as_query you document the
> bind values with a link to DBI, this is wrong. We do not take them in
> such format. The documentation for the format was never written either,
> but I wrote a very detailed explanation on the last refactor:
> https://github.com/dbsrgits/dbix-class/commit/0e77335

Errr, hmmmm, I might need a code example to work from.  Is @bind_values
an array of hashrefs?  I think I can handle the tech writing if I have enough
information.

> = 30d66bce
>
> Explain your motivation for ditching moniker. The nomenclature has been
> in use for a while, and is referenced in other modules.

Castaway suggested it, but I agree with her.  Part of this doc change
is trying to
standardize the language we use when we are referencing variable names.
The term "moniker" is just a fancy synonym for "name".  What name?  Row
name?  Result name?  Band name?  Dog name?

In this case, it's the source name, so why not just call it $source_name,
especially since we use the term source_name elsewhere.  Heck, the term
"source_name" is even an expected method name in result classes.  Why call
them two different things?

In fact, I should probably expand that change to remove the term everywhere.

> = df0207cb2c
>
> It reverts 30d66bce while doing other stuff on top... what? :)

Errr, okay, that was weird.  I must have saved Schema.pm while switching or
something.  Will correct.  There shouldn't be Schema.pm changes at all there.

> lib/ is for stuff that gets installed on the users machine.
> DBIx::Class::_PodInherit has no business there - move it to maint/

Well, I would argue the same for DBIC::OptDeps, but I've been told that it
has a public API.  Will move.

> The heredoc switching to single/double quotes makes no sense - you can
> use single quotes in a <<'HERE' just fine. Also perhaps instead of a
> heredoc your use warrants __DATA__ ?

So, you can use single quotes inside 'HERE' blocks?  That was my own
concern.  Though, I like the __DATA__ idea.

> = ce2fdc6b
>
> There is no point to have this commit on its own - please squash into
> 03bf20842

I can, but that happens before the ResultClass -> Manual::ResultClass
change.  I guess that doesn't really matter.

> === Nitpicks
>
> = 03bf20842
>
> You are using $result all over the place - consider switching to
> $result_obj. A $result may very well refer to a resultset (especially to
> the uninitiated who may expect multiple rows in a particular place).

We should be careful not to abuse the term '_obj'.  In this case, though,
I agree that it might be confusing without it.  (Though, we do have the link
in almost all of these places.)

> In ResultSet you left links to "rows/results" - it's kind at odds with
> the rest of your changes.

It is, but search is the most used function in DBIC, so I wanted to leave
a subtle reminder that the return values are basically just rows.  I did not
call them "row objects", which I -am- trying change.  I still called them
"@result_objs" in the return values.

> In Relationship::Base you say "his will return either a L<Result|..." -
> should be Result Object.

Yeah, that's where I'm getting iffy about the term "object" now.  If that
is a "Result Object", isn't the rest going to be a "ResultSet Object", or
"ResultSource Object" or "Thingy Object"?  IOW, is the object implied
here, or should it be clarified everywhere?

> (since I wrote this I saw that faba003 addresses it)
> In Relationship::Base::new_related "new item" should be replaced with
> "new result object". There are other instances of "new item" further down.

Castaway caught this, too, hence the commit update.

> In several methods you list "Return Value: 1" - it is probably wiser to
> list it as "not defined", since these are in effect undefined behaviors,
> and we may need to use them for something else down the road. Once you
> document them - this is it.

True; will correct.

> In Relationship::Base::delete_related you document the RV correctly, but
> left the odd varname in the source ($obj). Please change it to match.

Will correct.

> I am not entirely sure if changing Row to Result *wholesale* is a good idea,
> particularly in =headX sections. These are most possibly linked from other
> distributions and/or web resources. This is an "on-the-fence" issue, I am
> just bringing it up in case you have not thought of it.

IMHO, the benefits of less confusing docs outweighs the potential minor
problem of header link rot.

> (since I wrote this I saw that faba003 addresses it)
> In Manual::Glossary you describe prefetch as "like join but data is cached".
> This is not entirely correct as a plain join will never actually select
> anything, hence there is no data that prefetch would cache.

Castaway caught that one.

> Perhaps Manual::Reading should be split into Manual::Reading and
> Manual::Dev::Writing?

Probably.  I hinted at that in another msg thread.  That might be left for
a separate topic, though.

> In Row when explaining accessors you note it is recommended to use them
> because ... and state a weird reason. The reason to use accessors at all
> times is for proper encapsulation, so that CMM-style around()s can be
> safely written around an accessor. This is mst's theory anyway, ask for
> more clarification.

I dunno.  I think your reason is a lot weirder than mine, especially to the
layman.  (I don't even know what CMM is.)

> For the sake of `git blame`-ing in the future it would help to rebase your
> tree so that the changes to Optional::Dependencies, which afterwards migrated
> away in df0207cb2 are gone. If one is used to git this operation takes about
> a minute, but otherwise it may get rather tricky rather fast. Hence this is
> among nitpicks.

Yeah, it looks like I'm going to need to do a serial/edit/patch job, anyway,
so I might as well reorg the chunks here.

> = faba003ac
>
> There is nothing in this commit that warrants it standing alone. I would
> squash it into 03bf2084 (think of the `git blame`rs ;)
>
> = ce2fdc6b
>
> There is nothing in this commit that warrants it standing alone. I would
> squash it into 03bf2084 (think of the `git blame`rs ;)

Readability decisions.  Probably worth discussion in the AllHands meeting
tomorrow because I get confused by how you guys want the commits.
(Though, I actually saved 03bf2084 as-is to save on re-reading for when I
fixed the suggestions from IRC.)

-- 
Brendan Byrd <Perl at ResonatorSoft.org>
Brendan Byrd <BBYRD at CPAN.org>