Skip Menu |
 

This queue is for tickets about the Catalyst-Plugin-Authentication CPAN distribution.

Report information
The Basics
Id: 61178
Status: open
Priority: 0/
Queue: Catalyst-Plugin-Authentication

People
Owner: Nobody in particular
Requestors: perl [...] evancarroll.com
Cc:
AdminCc:

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



Subject: [PATCH] Moosification on C:P:A:Credential:Password
Download (untitled) / with headers
text/plain 509b
I'm going to need to get these upstream or I'm going to have to fork it. Thanks in advance, my cutoff date is going Oct 8, 2010. html: http://github.com/EvanCarroll/Catalyst-Plugin- Authentication/compare/f3848f59f08687623cee...133341ba8f45fecc3899 raw: http://github.com/EvanCarroll/Catalyst-Plugin- Authentication/compare/f3848f59f08687623cee...133341ba8f45fecc3899.patch Catch me on email, and CC the lists or unban me from IRC. -- Evan Carroll System Lord of the Internets http://www.evancarroll.com
Download (untitled) / with headers
text/plain 270b
You diff includes massive whitespace changes, which makes it pretty unreadable. Any chance you could prepare one which includes only actual differences and it'll be considered. Also, you probably want to adjust the Makefile.PL for dependency changes you've made. TIA.
CC: perl [...] evancarroll.com
Subject: Re: [rt.cpan.org #61178] [PATCH] Moosification on C:P:A:Credential:Password
Date: Wed, 8 Sep 2010 21:45:25 -0500
To: bug-Catalyst-Plugin-Authentication [...] rt.cpan.org
From: Evan Carroll <evan [...] dealermade.com>
Download (untitled) / with headers
text/plain 923b
Show quoted text
> You diff includes massive whitespace changes, which makes it pretty > unreadable. > > Any chance you could prepare one which includes only actual differences > and it'll be considered.
I'm not going to bother with whitespace because ultimately little code remains. It has with gone total moosification, and this code was probably written six month ago (when I first mentioned my github patch). Though, if needed I may consider upgrading the rest of C:P:A, I made an honest attempt to get the input required on this project many times, and the magnet brass thought it more important to keep the otherwise productive conversation off their irc network. Show quoted text
> Also, you probably want to adjust the Makefile.PL for dependency changes > you've made.
Good catch I'll get on that tomorrow morning.. -- Evan Carroll - evan@dealermade.com System Lord of the Internets - Dealermade web: http://www.dealermade.com ph: 888.403.9143
Download (untitled) / with headers
text/plain 835b
On Wed Sep 08 22:45:55 2010, evan@dealermade.com wrote: Show quoted text
> I'm not going to bother with whitespace because ultimately little code > remains.
There are 174 lines of code. I think saying that 'little remains' is entirely wrong. There are also feature changes without any tests for the new features. I'm aware that the test coverage here isn't amazing, but that isn't a reasonable excuse to not add some (and also to not add tests for bugs you introduced then fixed).. I'd also note that the class isn't closed (made immutable), your BUILDARGS method uses SUPER rather than next::method (and has unneeded logic), the namespace of the class isn't cleaned in any way (no no Moose etc), strict and warnings are still imported when not needed and the code layout has been changed to be entirely unlike all the other code in the module...
Show quoted text
> There are also feature changes without any tests for the new features. > > I'm aware that the test coverage here isn't amazing, but that isn't a > reasonable excuse to not add some (and also to not add tests for bugs > you introduced then fixed)..
Not, sure I know what bugs you're speaking of. Show quoted text
> I'd also note that the class isn't closed (made immutable), your > BUILDARGS method uses SUPER rather than next::method (and has unneeded > logic), the namespace of the class isn't cleaned in any way (no no
Moose Show quoted text
> etc), strict and warnings are still imported when not needed and the > code layout has been changed to be entirely unlike all the other code
in Show quoted text
> the module...
That logic comes straight from the Moose docs, it supports the legacy positional build args. No tests added, yet. The rest of the changes have been committed. No whitespace reformatting done - I've got my own style, I rewrote enough code to not be interested in analyzing and adopting a random style found on CPAN, if this is a deal-breaker that's ok. -- Evan Carroll System Lord of the Internets http://www.evancarroll.com


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.