Skip to content

Conversation

@szabgab
Copy link
Contributor

@szabgab szabgab commented Nov 2, 2014

No description provided.

@oalders
Copy link
Member

oalders commented Nov 9, 2014

@rwstauner or @haarg Would one of you like to review the JS in this pull request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could pare this down a bit by moving most of this logic into lib/MetaCPAN/Web/Role/Request.pm

Something like:

sub get_page_size {
    my $req = shift;
    my $default_page_size = shift;

    my $page_size = $req->param('size');
    unless ( is_PositiveInt($page_size) && $page_size <= 500 ) {
        $page_size = $default_page_size;
    }
    return $page_size;
}

The controllers should consume this role by default, so this would cut down on some boilerplate. If you're short on time, I can make these changes for you. Just let me know. I also see that you have different default page sizes, so we could deal with this here by suppl

@oalders
Copy link
Member

oalders commented Nov 9, 2014

I apologize that it has taken me so long to review the code. Thanks very much for taking this on. :)

@szabgab
Copy link
Contributor Author

szabgab commented Nov 9, 2014

Rebased in pull request #1417

@szabgab szabgab closed this Nov 9, 2014
@oalders
Copy link
Member

oalders commented Nov 10, 2014

Thanks for the refactor. BTW, if you just force-push your branch that's enough to update the current pull request. There's no need to open a new pull request, unless you have other reasons for wanting to go that route.

@szabgab
Copy link
Contributor Author

szabgab commented Nov 10, 2014

OK, I did not know that. Next time I'll try that.

@szabgab szabgab deleted the szabgab/pager branch November 12, 2014 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants