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

[Brendan Byrd]

On Tue, Dec 11, 2012 at 5:43 AM, Peter Rabbitson <rabbit+dbic at rabbit.us> wrote:
>
> Excellent, excellent work. Some pieces still need an extra touch, so I
> applied some as-is ans some commits got split. All of this is detailed
> below. The "leftovers" were pushed to a new branch
> topic/docfixes_pending.

Thanks.  I'll check out the new branch in a bit.

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

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

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

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

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

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

> * The signature of Relationship::Base add_to_$rel is plain wrong (while
> set_$rel is correct)

Will fix.

> * in Resultset::new() you do not link to \%attrs as everywhere else

Will fix.

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

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

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

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

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

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.  Or some sort of attribute that would force no
defaults.

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.

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.

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