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

[Peter Rabbitson]

On Mon, Dec 03, 2012 at 03:53:40PM -0500, Brendan Byrd wrote:
> This is the last of the leftovers from that rs_docs_fix2 branch that I've been
> trying to finalize.
> 
> Castaway already looked at the first commit, and gave me some other recommended
> changes.  So, I kept that commit and added others to ease the diff reading.  I
> also split out the commits that involved a lot of little changes.

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.

=== 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.

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

(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?

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).

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

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

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

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

= 30d66bce

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

= df0207cb2c

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

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

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__ ?

= ce2fdc6b

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

=== 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).

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

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

(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.

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.

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

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.

(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.

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

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.

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.

= 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 ;)