Skip Menu |
 

This queue is for tickets about the Data-Page CPAN distribution.

Report information
The Basics
Id: 130686
Status: open
Priority: 0/
Queue: Data-Page

People
Owner: Nobody in particular
Requestors: ribasushi [...] leporine.io
Cc:
AdminCc:

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



Subject: Check for "accessor takes only integers" breaks subclassing
Download (untitled) / with headers
text/plain 388b
The check introduced here: https://github.com/karenetheridge/Data-Page/commit/70286c7fb2f75dd7239c6adb6d1f602964d26299#diff-2b75d6496b291502e446db7dd34f8426R54 Breaks the following subclassing: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/lib/DBIx/Class/ResultSet/Pager.pm If the checks are there to stay, please consider making them standalone methods that can be noop-ed.
Download (untitled) / with headers
text/plain 534b
On 2019-10-10 07:59:35, RIBASUSHI wrote: Show quoted text
> The check introduced here: https://github.com/karenetheridge/Data- > Page/commit/70286c7fb2f75dd7239c6adb6d1f602964d26299#diff- > 2b75d6496b291502e446db7dd34f8426R54 > > Breaks the following subclassing: > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > 0.082841/lib/DBIx/Class/ResultSet/Pager.pm > > If the checks are there to stay, please consider making them > standalone methods that can be noop-ed.
I don't see the problem. Is something calling total_entries with @_ = (undef) ?
Download (untitled) / with headers
text/plain 293b
On 2019-10-11 20:15:06, ETHER wrote: Show quoted text
> I don't see the problem. Is something calling total_entries with @_ = > (undef) ?
Ok I see the situation now. I had to unwind a few levels and find how DBIx::Class::ResultSet was constructing the pager object. I should be able to work something out.
Download (untitled) / with headers
text/plain 646b
On Sat Oct 12 05:20:55 2019, ETHER wrote: Show quoted text
> On 2019-10-11 20:15:06, ETHER wrote: >
> > I don't see the problem. Is something calling total_entries with @_ = > > (undef) ?
> > Ok I see the situation now. I had to unwind a few levels and find how > DBIx::Class::ResultSet was constructing the pager object. > > I should be able to work something out.
_05 does fix the priginally reported problem, but this change https://github.com/karenetheridge/Data-Page/commit/49529b87a1 breaks the entire concept of lazily-evaluated-total-count. If this behavior will stay as-is, I need about 2 weeks to fully prepare/test/release a workaround for DBIC.
Download (untitled) / with headers
text/plain 528b
On 2019-10-12 07:31:00, RIBASUSHI wrote: Show quoted text
> _05 does fix the priginally reported problem, but this change > https://github.com/karenetheridge/Data-Page/commit/49529b87a1 breaks > the entire concept of lazily-evaluated-total-count. > > If this behavior will stay as-is, I need about 2 weeks to fully > prepare/test/release a workaround for DBIC.
What would you suggest? I would switch to Moo + some XS type checks from Type::Tiny, except that might break something else upstream that depended on the internal accessor methods.
Download (untitled) / with headers
text/plain 684b
Current subject: "Reorder of operations breaks existing subclass assumptions" Show quoted text
> On 2019-10-12 07:31:00, RIBASUSHI wrote: > > ... this change https://github.com/karenetheridge/Data-Page/commit/49529b87a1 breaks the entire concept of lazily-evaluated-total-count
Show quoted text
> On Sat Oct 12 18:15:06 2019, ETHER wrote: > > ... I would switch to Moo + some XS type checks from Type::Tiny...
I do not know how to respond to your suggestion as it focuses on operation implementation. You changed the *ORDER* of operations. My question is twofold: - Are you planning to revert this? - If not - do I have until ~Oct 27th to make a DBIC-side fix for this, or do I need to scramble something ASAP?
Download (untitled) / with headers
text/plain 893b
On 2019-10-13 09:11:52, RIBASUSHI wrote: Show quoted text
> Current subject: "Reorder of operations breaks existing subclass > assumptions" >
> > On 2019-10-12 07:31:00, RIBASUSHI wrote: > > > > ... this change https://github.com/karenetheridge/Data- > > Page/commit/49529b87a1 breaks the entire concept of lazily-evaluated- > > total-count
>
> > On Sat Oct 12 18:15:06 2019, ETHER wrote: > > > > ... I would switch to Moo + some XS type checks from Type::Tiny...
> > > I do not know how to respond to your suggestion as it focuses on > operation implementation.
I am asking what *you* would prefer to do. Were any of the changes in 2.04 significant (in a positive way) to you? Show quoted text
> - If not - do I have until ~Oct 27th to make a DBIC-side fix for this, > or do I need to scramble something ASAP?
What is the significance of that date? I had no specific timeline for moving the trial changes to stable.
Download (untitled) / with headers
text/plain 865b
On Sun Oct 13 18:39:31 2019, ETHER wrote: Show quoted text
> > I am asking what *you* would prefer to do. >
As a consumer I prefer platform-type stuff to not change from under me. On the other hand the use-case of DBIC is *very* unusual, so I do not have a good position to advocate what happens to the future of Data::Page. Show quoted text
> Were any of the changes in 2.04 significant (in a positive way) to > you?
This is a loaded question, I rather not answer it. Show quoted text
> What is the significance of that date? I had no specific timeline for > moving the trial changes to stable.
Tentatively I will have time before that date to look into a workaround on my side, so that future changes in Data::Page won't be a concern. I am simply reporting a bug and inquiring what are my options. I rather not go down the path of near-synchronous dialogue - just let me know what are you planning to do.
Download (untitled) / with headers
text/plain 865b
On 2019-10-13 10:15:26, RIBASUSHI wrote: Show quoted text
> > Were any of the changes in 2.04 significant (in a positive way) to > > you?
> > This is a loaded question, I rather not answer it.
It was not loaded. I was asking if any of those changes are important to you -- that is, if I rolled back everything, would there be more problems. Show quoted text
> I am simply reporting a bug and inquiring what are my options. I > rather not go down the path of near-synchronous dialogue - just let me > know what are you planning to do.
I will not release changes to the PAUSE index that are known to be breaking an important downstream module. After that, I am trying to find the right course forward, but your terse responses are not helping. At this point I do not understand what the problem is with 49529b87a; I have not been able to reproduce any issues with DBIx::Class::ResultSet::Pager.
Download (untitled) / with headers
text/plain 2.4k
On Sun Oct 13 20:40:40 2019, ETHER wrote: Show quoted text
> On 2019-10-13 10:15:26, RIBASUSHI wrote: >
> > > Were any of the changes in 2.04 significant (in a positive way) to > > > you?
> > > > This is a loaded question, I rather not answer it.
> > It was not loaded. I was asking if any of those changes are important > to you -- that is, if I rolled back everything, would there be more > problems. >
There would be no problems rom my perspective. Though again - the DBIC use case is very obscure, I rather not have the Data::Page changes be held back by it. I just need a few weeks. Show quoted text
> > I am simply reporting a bug and inquiring what are my options. I > > rather not go down the path of near-synchronous dialogue - just let > > me > > know what are you planning to do.
> > I will not release changes to the PAUSE index that are known to be > breaking an important downstream module.
Excellent. Show quoted text
> > After that, I am trying to find the right course forward, but your > terse responses are not helping. At this point I do not understand > what the problem is with 49529b87a; I have not been able to reproduce > any issues with DBIx::Class::ResultSet::Pager.
I made several bad assumptions based on reading https://github.com/karenetheridge/Data-Page/commit/02456a21505a8e7cbc67d34b7911356c2d5a79a6#diff-7d8a9cef418ee48430e98051fff20c8a . Specifically: - The fact that you fixed the bug implied that you understood what the coderef piece is for in the first place - Adding DBIC as an optional test-prereq implied that you ran DBIC's entire test suite against the new version of Data::Page. Now that I know this hasn't happened - here is the gist: - Previously all current_page() checks happened at get-time, which allowed for parts of the stack to be "lazified" - This was (ab)used to skip an expensive `SELECT COUNT(*)...` when the user cares about the first page ( 99% of the cases ) - The change you made forces the sanity check to happen at *object construction time*, which fires the coderef nullifying all lazyfication by virtue of current_page($foo) => last_page() => total_entries() To be even more specific, this block is now impossible to satisfy: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/t/67pager.t#L19-28 With all that said, I again must stress that this is probably the only case of lazification in the wild. Thus having DBIC part ways with Data::Page is likely the correct answer. It isup to you how this ticket gets resolved ( revert or won't-fix ), I should have my side squared away in couple weeks.
Download (untitled) / with headers
text/plain 2.1k
On 2019-10-15 05:54:51, RIBASUSHI wrote: Show quoted text
> I made several bad assumptions based on reading > https://github.com/karenetheridge/Data- > Page/commit/02456a21505a8e7cbc67d34b7911356c2d5a79a6#diff- > 7d8a9cef418ee48430e98051fff20c8a . Specifically: > - The fact that you fixed the bug implied that you understood what the > coderef piece is for in the first place > - Adding DBIC as an optional test-prereq implied that you ran DBIC's > entire test suite against the new version of Data::Page. > > Now that I know this hasn't happened - here is the gist: > - Previously all current_page() checks happened at get-time, which > allowed for parts of the stack to be "lazified" > - This was (ab)used to skip an expensive `SELECT COUNT(*)...` when the > user cares about the first page ( 99% of the cases ) > - The change you made forces the sanity check to happen at *object > construction time*, which fires the coderef nullifying all > lazyfication by virtue of current_page($foo) => last_page() => > total_entries() > > To be even more specific, this block is now impossible to satisfy: > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > 0.082841/t/67pager.t#L19-28
Sorry about that. The test that I added was clearly incomplete and I didn't re-run DBIx::Class's tests after I made the change. I see the issue now. Show quoted text
> With all that said, I again must stress that this is probably the only > case of lazification in the wild. Thus having DBIC part ways with > Data::Page is likely the correct answer.
Perhaps inlining the calculations into DBIx::Class::ResultSet::Pager might be best (or at least allow for more fine-tuning of hot code paths). That's fine with me as DP's underlying use of accessors is not that great, as you have seen as I have tried to wrestle with improving it :) Show quoted text
> It isup to you how this ticket gets resolved ( revert or won't-fix ), > I should have my side squared away in couple weeks.
Unless there is an emergency I may not be able to release a fix for this in the next week however. It sounds like you aren't in a position to get a fix out ASAP either, so that's good for both of us. I'm happy to coordinate schedules on irc. Let me know what you need.
Download (untitled) / with headers
text/plain 327b
On Tue Oct 15 20:58:42 2019, ETHER wrote: Show quoted text
> > I'm happy to coordinate schedules on irc. Let me know what you need.
Another trial has shipped no longer depending on Data::Page. Seems good so far. Stable should be another ~2 weeks away. Please keep the "eager" version of Data::Page unreleased until I add a "go-ahead" here.


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.