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

[Peter Rabbitson]

On Tue, Dec 04, 2012 at 12:05:59AM -0500, Brendan Byrd wrote:
> 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 :)

If it is not in the quotes below - I either withdrew my concern or you
said "I will correct it":

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

Oh I should have executed it to see what exactly it does. Right - then this
point splits into several problems:

- In this case (number of files) we definitely can not do it in the workdir,
sorry. There is no easy way to declutter things (make realclean does not touch
them), and files will linger forever in a repo even if the underlying .pm is
gone/renamed. Here is a scetch of a functional workaround, will need however
get to CPAN as a devrel for render-testing:
https://github.com/dbsrgits/dbix-class/commit/38a597851

- I would strongly recommend not generating POD for the ::Storage family - 
this entire lot need a massive rework, and some things may disappear. Your
call though.

- PI needs a way to parse the target POD and exclude methods which are 
undocumented at their origin. And yes, there is a number of "public" 
(non _ed) methods which are in fact undocumented. Maybe they are utility 
(e.g. STORABLE_freeze) or maybe they are deprecated warning-emitting 
methods, which are nevertheless still there (and will be there for a 
while). I am not that concerned over dead links, I am much more 
concerned about re-exposure of hidden stuff.

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

But before the start of your branch none of these links existed. Hence 
if squashed together the resulting commit will not differ in size. We 
can discuss more of this tonight.

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

+1
 
> > 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.

Then let's do just that and fix ::Reading. As a separate commit, either 
before or after the current set of changes (before would be more 
sensible).

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

Array of tuple-arrays. Look at any test changed by the commit I 
mentioned above - they all follow the same principle. I was really under 
the impression the commit message was rather exhaustive from an 
explanation POV. In any case - ask as many questions as you need - we 
have not had this documented for 2 years now, it's time.

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

I am on the fence. I would run this particular bit by mst, see what he thinks.

> > 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 *main* purpose of DBIC::OptDeps is to be used by *downstream deps* 
of DBIC. Hence - yes it has a very public API. Perhaps The docs need 
work to make this clearer. Note the SYNOPSIS: 
https://metacpan.org/module/DBIx::Class::Optional::Dependencies#SYNOPSIS 
Basically instead of having to track the minimum version of an optional 
DBIC dep, and re-relasing your module every time DBIC bumps it - you 
just ask DBIC itself and move on.

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

Correct - once you declare a literal heredoc with <<'FOO', nothing short
of /^FOO$/m or EOF will terminate it.

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

I don't have a particular opinion on _obj. My concern is that a bare 
'result' is something we need to avoid, as it is way too generic. I'd 
like others to chime in on that. Castaway?

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

Perhaps a more explicit '... (formerly called rows or row objects)...'
sprinkled around?

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

See discussion of 'result' above. Also yes, having the rest say 
ResultSet Object won't be a bad idea imo. But this is even more work, we 
better first agree on a path forward.

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

We should probably start that if not too difficult. Will reduce the amount
of "holy shit where do I start", and will clearly delineate the ::Dev::
namespace.

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

Not mine actually - I am not shy to use get_column when I believe it is 
warranted. Hence ask mst/abraxxa about it.