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

[Peter Rabbitson]

On Tue, Dec 11, 2012 at 11:01:42AM -0500, Brendan Byrd wrote:
> > Also in 2b3ac681 (which is already applied) you based the pod of
> > DBIx::Class::Manual::ResultClass on earlier work of mine found in
> > 14f8d7dbc. However you left out the initialization docs - please
> > add them back in a subsequent commit.
> 
> Oh, I thought these were inherited methods, but I guess you're trying
> to describe what to put into a result class.  Yeah, I'll add them back
> in on a different commit.

Somewhat - it is all for your consideratin only - perhaps things can
stay as-is.

> 
> > Also if PI allows it, it may
> > be nice to separate the ResultSource::Proxy inheritance from
> > everything else, the way I planned to do it in 14f8d7dbc. In any
> > case this details further work - the actual stuff is already in
> > master.
> 
> Not sure I follow.  Why would RS:P be separated?  I thought what you
> had in there was some preliminary docs on how to get the result class
> up and going.

Look at 14f8d7dbc - it has two sections (well three if you count the one
from above):

=head1 INITIALIZATION METHODS

=head1 RESULTSOURCE METADATA METHODS

=head1 DATA INSTANCE MANIPULATION METHODS

What I meant was - perhaps it is possible to separate the bunch of
inherited methods into separate =head's. Same caveat as above applies -
it is just a "nice to have"

> 
> > topic/rs_doc_fix2 - an older branch of yours - seems to contain nothing
> >   that has not been already done - review and delete
> 
> Yes, I planned on requesting a delete on a few branches after I got
> everything put in place and confirmed that they were no longer needed.

These are your branches - if they seem no longer needed:
git push origin :topic/rs_doc_fix2

> 
> > === 2b3ac68
> > The first commit 2b3ac68 was applied with very minor changes:
> > `git diff --color 2b3ac68 3d4c5a8`
> 
> # if the author doesn't have them, don't fail the initial "perl
> Makefile.pl" step
> 
> You kept the comment here, but changed the code so that it wouldn't
> eval.

A do EXPR *is* an eval on its own. In fact doing eval{ do Foo.pm } will
reliably hide $@ from you ;)

> I don't remember if non-authors would end up in the 54_*.pl
> file, but they might see the error message.  If 54_*.pl is ran by
> non-authors, we would need to hide the error from them.

No, entirety of maint/Makefile.PL.inc is author-only:
https://github.com/dbsrgits/dbix-class/blob/master/Makefile.PL#L131

> Also, gen_pod_inherit will print everything to standard out, so they
> would likely see the error messages, anyway.  (I know this is the case
> for the debugs.)

It will never get invoked in the 1st place.

> 
> > === 0cab3cc
> > 0cab3cc30 still has a number of problems so I applied it *partially*.
> > Everything was taken as-ise, except for Relationship::Base and Resultset,
> > which were not applied at all. I re-pushed your branch with the leftovers.
> > The problems that need fixing are as follows:
> >
> > * In Relationship::Base you normally link \%attrs, however you failed to do
> > so in update_or_create_related (you only mention key => ). Also above it in
> > find_or_create_related you *do not* mention key =>. Please adjust.
> 
> So, replace "{ key => $unique_constraint }" with \%attrs (plus the link)?
> 
> Yeah, I see the original docs with that.  Not sure what happened
> there.  Will fix.

Well not quite - key is a *special* attribute, and as such is not documented
in Resultset. Perhaps something like:
{ key => $unique_constraint_name, %attrs(+link) }?

> > * in Resultset $cond has inconsistent linking to SQLMaker (sometimes it
> > does sometimes it doesn't). I am also not sure if linking to SQLMaker is
> > of any value... especially given the DQ underpinnings change. Perhaps not
> > link for the time being at all, or add a "search arguments" document
> > somewhere in ::Manual::, which in turn links to the appropriate resources
> > of the day?
> 
> I wouldn't exactly say that SQLMaker is inappropriate, but it should
> be consistent.  (I'll check the consistency issue.)  SQLMaker directly
> links to SQL:A, and explains what it does.
> 
> As far as DQ, that's not quite up and running yet, and it should match
> SQL:A's structure (mostly), anyway.  When DBIC starts using DQ, we can
> change the links then.

Right, I was mostly pointing to the "search arguments" section somewhere.
Again - just a nice-to-have.

> > * Resultset::update_or_create has the same problem with key/attrs as above
> 
> There must have been some changes in the past six months since this
> patch first started.  I'll fix these, and hunt down any other weird
> "key => $unique_constraint" references.

Not that I recall... but anyhow - see above comment about 'key' not being
exactly a resultset attr.

> 
> > === dc26bfc45
> > Applied partially - the [ $val ] === [ {}, $val ] part is not as simple
> > as you may think. Consider:
> >
> > perl -Ilib -It/lib -MDBICTest -e 'DBICTest->init_schema->resultset("Artist")->search({ name => \["=", [ "foo" ] ]})->as_query'
> > DBIx::Class::ResultSet::as_query(): [SQL::Abstract::_assert_bindval_matches_bindtype] Fatal: bindtype 'columns' selected, you need to pass: [column_name => bind_value] at -e line 1
> >
> > Basically binds are interrogated within SQLA way before they make it to
> > bind-time in ::Storage::DBI. So I took this part away and left the rest
> > of the docs in.
> 
> Ahh, okay.  I was searching the DBIC code for any other references,
> but didn't look at SQLA.  Also, the test for this seemed to be a bit
> more challenging than putting it in.  (Would it go into
> sqlmaker/bind_transport.t?  Wasn't sure how to add things in without
> breaking that whole test.)

Honestly I would just write a new test for this based on one of the existing
ones.
> 
> > Also you had incorrectly linked the BIND documentation to search_literal -
> > I fixed this in d4a9feb6.
> 
> Wait, so the @bind there is a DBI (normal) style bind?  If so, I
> probably need to link to the original DBI docs (like I was doing), and
> make the variable name "stand out", like @dbi_bind.

Right - see how search_literal is actually implemented. Its entire purpose
is to take "bare values". Hence also why the deprecation.

> > === 7a82dc29
> > This was not applied - any documentation of default_attributes must be
> > combined with a massive warning of the caveats of doing so. Every time
> > someone asks "how do I set default attrs", they come back a day later
> > and ask "but how do I create a resultset just *this one time* that does
> > not have my default attrs (the answer is - you can't). Hence an
> > alternative approach of project-local methods that return new resultsets
> > with the desired attributes is always preferred.
> 
> This was a last minute addition based on this conversation from a
> user: https://gist.github.com/c8ea5ca3f4b3f941e32b

Right - and as you can see once used (say to add a prefetch) there is no
way to "undo" it. Hence why me asking for an explicit caveat.

> I'm also in favor of expanding resultset_attributes to fix whatever
> issues might arise from it.  Like deep nested hash combining, if a
> second option is given.

I don't follow - what do you mean by "nested hash combining"?

> Or some sort of attribute that would force no defaults.

That would be a solution while looking for a problem :) result_attributes
was badly thought through in the first place, and parts of it don't even
work properly (and are a massive bitch to fix). See RT#63709 for example.
The less the docs and existing machinery encourage their use - the better.

> In the meantime, we can still put in the docs with the caveats.  Or
> just put the caveats in the resultset_attribute docs, since there
> isn't anything in there about the pitfalls.  Users should have the
> power of greatest flexibility with the most knowledge available to
> them.  If there is something that might have pitfalls, we should
> detail those, not bury the option.

Ideally we should do both. Realise that a lot of the DBIC API was put in 
place by folks not overly concerned with the big picture. Yes, we can't 
remove them anymore, but discouraging stuff that is *architecturally* 
unsound is never a bad idea. But in general I agree with you - the more 
of this is documented the better. I am just arguing the direction this 
documentation should take (bury vs promote).

Perhaps someone else can chime in here?

> I'm probably going to look for more things to link back to other docs
> at some point, as there's a need for multiple connection points
> between documented methods.

+1. The more the better. There is also a maint/gen_pod_index which has
not been touched/used by anyone since 2006 (55a55d31). Perhaps it can
be brought back in service?

Cheers