Skip Menu |
 

This queue is for tickets about the DBIx-Class CPAN distribution.

Report information
The Basics
Id: 83712
Status: open
Priority: 0/
Queue: DBIx-Class

People
Owner: Nobody in particular
Requestors: MITHALDU [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: RFC: rename belongs_to to refers_to
Download (untitled) / with headers
text/plain 1.2k
Resulting out of an IRC discussion i created the branch "belongs_to_rename". It contains multiple commits that rename belongs_to to refers_to, adds an alias from belongs_to to refers_to; then renames all occurences of belongs_to in the code, documentation and tests. All these actions are split up into multiple commits to allow an easier review of the changes and to make it easier to compose the final commit after it's been decided how the change is desired to happen. I've not written any tests yet because the test suite is rather daunting and i'd like some guidance as to what should be tested and how. One possible way would be to skip the renaming of belongs_to in tests, as those tests would then test both methods. That seems to be kind of ghetto on the other hand though, with a test that belongs_to executes refers_to seeming more sensible. I am however unclear as to how that'd best be done. Suggestions welcome. As for the alias of belongs_to to refers_to, my original notion was simply to do `*belongs_to = \&refers_to;`, however that caused `55namespaces_cleaned.t` to complain about mismatching names of function and GV. Thus i solved it with `goto \&refers_to`. If that is not desired, suggestions welcome.
Subject: Re: [rt.cpan.org #83712] RFC: rename belongs_to to refers_to
Date: Sat, 2 Mar 2013 03:18:42 +1100
To: Christian Walde via RT <bug-DBIx-Class [...] rt.cpan.org>
From: Peter Rabbitson <ribasushi [...] cpan.org>
Download (untitled) / with headers
text/plain 2.9k
On Fri, Mar 01, 2013 at 11:03:19AM -0500, Christian Walde via RT wrote: Show quoted text
> Fri Mar 01 11:03:18 2013: Request 83712 was acted upon. > Transaction: Ticket created by MITHALDU > Queue: DBIx-Class > Subject: RFC: rename belongs_to to refers_to > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: MITHALDU@cpan.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=83712 > > > > Resulting out of an IRC discussion i created the branch > "belongs_to_rename". It contains multiple commits that rename > belongs_to to refers_to,
I need to start with a disclaimer - I do not want to demotivate you, this is something that we need to do for a very long time (the renaming). Yet your proposal suffers a semantical problem. Please read the rest as an attempt at objective criticism, not as an attempt to kill your effort outright. This has come up many times in the past, and this patch *also* falls short of addressing the main problem. You are looking at belongs_to as an isolated method. Yet before we had: belongs_to / might_have / has_one / has_many now we have refers_to / might_have / has_one / has_many While before folks would get confused what might_have and has_one means, at least they would always get belongs_to correct (due to the implied parent/child relation). I believe the new naming scheme makes things even worse as the only thing not ambiguous now is "has_many". Your proposal would be much more complete if it went refers_to / maybe_refers_to / referred_to / maybe_referred_to / many_refer_to But it results in weird english. I proposed a scheme a long time ago, which is not glamorous but is *uanmbiguous*: owned_by / maybe_owned_by / owns_one / maybe_owns_one / owns_many This proposal was not supported sadly. Show quoted text
> I've not written any tests yet because the test suite is rather > daunting and i'd like some guidance as to what should be tested and > how.
I would provide help (or do the tricky parts myself, as backcompat is ugly business to test). But we need to get saner names first (or have most of the cabal agree with your current proposal) Show quoted text
> As for the alias of belongs_to to refers_to, my original notion was > simply to do `*belongs_to = \&refers_to;`, however that caused > `55namespaces_cleaned.t` to complain about mismatching names of > function and GV. Thus i solved it with `goto \&refers_to`. If that is > not desired, suggestions welcome.
You may never do this in OO programming. Because if someone is overriding the refers_to method, and then another user calls the belongs_to proxy, perl will dispatch to the original refers_to, not the override. You always do: sub old_name { shift->new_name(@_) } Furthermore the exceptions need to be doubled up, to throw text containing 'belongs_to' when the exception is a result of calling a 'belongs_to'. A 'refers_to' containing error message in an old codebase, where refers_to does not exist will be very confusing.
Subject: Re: [rt.cpan.org #83712] RFC: rename belongs_to to refers_to
Date: Mon, 04 Mar 2013 14:45:10 +0100
To: "Peter Rabbitson via RT" <bug-DBIx-Class [...] rt.cpan.org>
From: "Christian Walde" <walde.christian [...] gmail.com>
Download (untitled) / with headers
text/plain 5.1k
On Fri, 01 Mar 2013 17:19:00 +0100, Peter Rabbitson via RT <bug-DBIx-Class@rt.cpan.org> wrote: Show quoted text
>> Resulting out of an IRC discussion i created the branch >> "belongs_to_rename". It contains multiple commits that rename >> belongs_to to refers_to,
> > I need to start with a disclaimer - I do not want to demotivate you, > this is something that we need to do for a very long time (the > renaming). Yet your proposal suffers a semantical problem. Please read > the rest as an attempt at objective criticism, not as an attempt to kill > your effort outright.
No worries. I made those changes mostly to see what they'd end up being, fully aware that they'll likely be partly or fully scrapped. Show quoted text
> This has come up many times in the past, and this patch *also* falls > short of addressing the main problem. You are looking at belongs_to as > an isolated method.
I'm doing so because that was how far i got in trying to use DBIC. To explain, i have a table representing PPI elements, which i deploy() to an empty sqlite database, which looks like this: id | type | source | line | column | long_pattern | short_pat .. | struc | ...[A] | ... | ... | ...[A] | ...[A] .. | block | ...[A] | ... | ... | ...[A] | ...[A] .. | struc | ...[B] | ... | ... | ...[B] | ...[B] .. | struc | ...[C] | ... | ... | ...[C] | ...[B] .. | block | ...[D] | ... | ... | ...[D] | ...[C] .. | block | ...[E] | ... | ... | ...[D] | ...[C] I'd like to remove duplication, by normalizing into something like this: id | type | source | loc | long_pattern | short_pat .. | struc | 1 | 1 | 1 | 1 .. | block | 1 | 1 | 1 | 1 .. | struc | 2 | 2 | 2 | 2 .. | struc | 3 | 3 | 3 | 2 .. | block | 4 | 4 | 4 | 3 .. | block | 5 | 5 | 4 | 3 Leaving out all but the source column, for now, my naive thought was: Each element has one source string. Different elements might share the same, but each has one. Turned out has_one was wrong for that though, it should've been belongs_to (as far as functionality goes) However, the source column is just metadata that i'd normally even throw away and only keep around so i can print nicer reports about the harvested data down the line. They're only separate because of memory concerns. In fact, the elements belong so little to the source bits that the elements *need* to be kept around even if the source bit is removed from the database. The same would be true for location. In some analysis cases i would not care at all where the element was located. What happens here is that each element simply has a reference to a bit of metadata. In these two cases belongs_to does not accurately reflect the type of relationship. That is why, for now, i was only looking at belongs_to. In the meantime i've had more time to think about it, which is partly why my table up there contains two more columns. Each of the elements will be compressible, via different algorithms, into code patterns. These are not simply metadata for my purposes, they actually stand for groups of things. And each element belongs to one group and each element is only useful if it belongs to these groups. As such, for those relationships belongs_to is perfectly correct. So, as far as i can tell (and i might be entirely wrong due to my inexperience) there are two semantically different cases that are implemented by belongs_to() in the same manner. So right now i'm thinking that both need to be around. Show quoted text
> other relationships
I'm sorry, i still have not gotten that far (aside from being told to never use has_one) and cannot useful opine on those. I will need to actually read the documentation before doing so, but did not wish to leave you waiting for a reply until i've done so. Show quoted text
> owned_by / maybe_owned_by / owns_one / maybe_owns_one / owns_many
Looking at those i'd choose maybe_owns_one for location and source, and owned_by for the pattern groups, expecting DBIC to generate the table layout above. Is that what you're thinking? Show quoted text
>> As for the alias of belongs_to to refers_to, my original notion was >> simply to do `*belongs_to = \&refers_to;`, however that caused >> `55namespaces_cleaned.t` to complain about mismatching names of >> function and GV. Thus i solved it with `goto \&refers_to`. If that is >> not desired, suggestions welcome.
> > You may never do this in OO programming. Because if someone is > overriding the refers_to method, and then another user calls the > belongs_to proxy, perl will dispatch to the original refers_to, not the > override. You always do: > > sub old_name { shift->new_name(@_) }
Noted. I thought about this, but did not like it due to the performance impact. If we can agree on names, i will do that. Show quoted text
> Furthermore the exceptions need to be doubled up, to throw text > containing 'belongs_to' when the exception is a result of calling a > 'belongs_to'. A 'refers_to' containing error message in an old codebase, > where refers_to does not exist will be very confusing.
Ah, i was looking at that, but wasn't sure if it was necessary or not. -- With regards, Christian Walde
Subject: Re: [rt.cpan.org #83712] RFC: rename belongs_to to refers_to
Date: Tue, 5 Mar 2013 01:38:12 +1100
To: Christian Walde via RT <bug-DBIx-Class [...] rt.cpan.org>
From: Peter Rabbitson <ribasushi [...] cpan.org>
Download (untitled) / with headers
text/plain 3.7k
On Mon, Mar 04, 2013 at 08:45:28AM -0500, Christian Walde via RT wrote: Show quoted text
> So, as far as i can tell (and i might be entirely wrong due to my > inexperience) there are two semantically different cases that are > implemented by belongs_to() in the same manner. > > So right now i'm thinking that both need to be around. >
> > other relationships
> > I'm sorry, i still have not gotten that far (aside from being told to > never use has_one) and cannot useful opine on those. I will need to > actually read the documentation before doing so, but did not wish to leave > you waiting for a reply until i've done so. >
> > owned_by / maybe_owned_by / owns_one / maybe_owns_one / owns_many
> > Looking at those i'd choose maybe_owns_one for location and source, and > owned_by for the pattern groups, expecting DBIC to generate the table > layout above. Is that what you're thinking?
Thank you for keeping on top of this. Your inexperience with the API is very very valuable to make progress where none has been made for years. I need to step back in order to explain this properly. Under the hood there is only one way to actually add a relationship in DBIC: https://metacpan.org/module/DBIx::Class::ResultSource#add_relationship The calls to belongs_to/has_many/has_one/might_have are simply convenience wrappers to assemble the condition and to set the correct relationship attributes (they are not all documented where I linked you, we suck, look here instead: https://metacpan.org/module/DBIx::Class::Relationship::Base#add_relationship). The way to differentiate them is to take the following attribute matrix: - join_type: inner OR left (right join makes little sense under the root->leaf design of DBIC) - is_multi: is it a 1:1 or a 1:M - is_dependent: 1/0 - the easiest way to explain this is the answer to the question: If this relationship had a FK constraint declared in the RDBMS - which way would it "point"? (the FK constraint obviously is not required, the relationship express an arbitrary JOIN clause, not RDBMS structure). Or in other words do we or does the far side hold values that can not exist without identical values on the other side of the relationship. Given the above, all our relationship helpers can simply be described as: belongs_to => { join_type => 'inner', is_multi => 0, is_dependent => 1 } has_one => { join_type => 'inner', is_multi => 0, is_dependent => 0 } might_have => { join_type => 'left', is_multi => 0, is_dependent => 0 } has_many => { join_type => 'left', is_multi => 1, is_dependent => 0 } As you can see above 'has_one' is kinda useless as an idea, hence why someone told you never to use it. It has merit as a use-case, but would be extremely rare. We currently do not have a 'might_belong_to' - this is probably what confused you. Also in some examples you see a belongs_to declaration with an overriden join_type - the "official" <puke> way of doing it currently. In this light the other naming proposal should be clearer. It should also be clear why I am opposed to changing *only* the name of belongs_to - as it stands currently that's the only helper that actually does what it says on the tin. Everything else requires repeated "aha" moments to get right. I would argue that even "has_many" is a tad misleading, because it does not convey explicitly the presence of a "left" join. Now given all of this extra info - what do you think? Also transcribing this into POD additions would be very very very much appreciated. Darned ETIME :( Show quoted text
> > sub old_name { shift->new_name(@_) }
> > Noted. I thought about this, but did not like it due to the performance > impact.
You are correct - there is a performance impact. There is no way of getting around this, short of designing proper API's that need no changes in the first place ;) Cheers
Subject: Re: [rt.cpan.org #83712] RFC: rename belongs_to to refers_to
Date: Mon, 04 Mar 2013 18:01:47 +0100
To: "Peter Rabbitson via RT" <bug-DBIx-Class [...] rt.cpan.org>
From: "Christian Walde" <walde.christian [...] gmail.com>
Download (untitled) / with headers
text/plain 1.9k
On Mon, 04 Mar 2013 15:38:39 +0100, Peter Rabbitson via RT <bug-DBIx-Class@rt.cpan.org> wrote: Show quoted text
> Now given all of this extra info - what do you think?
That you're going a bit further than i am and too much into the guts. I care about two things at the moment: 1. Does the class i set up generate the right table when i deploy it? (Not even thinking about fk, just elemental question of: Is the column i want where i want it?) 2. Does it have a useful accessor? (I don't really care how it's implemented, as i haven't looked at how accessors for relations work at all.) Your email, while very interesting (and successful in prompting me to read up on SQL again), did not really help me understand these issues about the helpers. Show quoted text
> It should also be clear why I am opposed to changing *only* the name of > belongs_to - as it stands currently that's the only helper that actually > does what it says on the tin.
Hmmm, i thought i'd demonstrated how for me its tin inscription did not match the result and how that only changed after considerable thinking and a change in view angle? :) Show quoted text
> Also transcribing > this into POD additions would be very very very much appreciated. Darned > ETIME :(
Not right now, but when i've a better grip on things i'll be sure to do doc patches. Show quoted text
>> > sub old_name { shift->new_name(@_) }
>> >> Noted. I thought about this, but did not like it due to the performance >> impact.
> > You are correct - there is a performance impact. There is no way of > getting around this, short of designing proper API's that need no > changes in the first place ;)
This is off the wall, but how about more simple Moo-style relation declarators? e.g.: package Source; refby element = ( as => 'leaf', multi => 1 ); package Element; refs source => ( as => 'root' ); refs pattern_short => ( as => 'leaf' ); package PatternShort; refby element => ( as => 'root', multi => 1 ); -- With regards, Christian Walde


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.