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)

History Show all quoted text
  Wed Sep 08 15:25:58 2010 perl [...] evancarroll.com - Ticket created
Subject: [PATCH] Moosification on C:P:A:Credential:Password
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
  Wed Sep 08 18:15:11 2010 bobtfish [...] bobtfish.net - Correspondence added
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.
  Wed Sep 08 18:15:14 2010 The RT System itself - Status changed from 'new' to 'open'
  Wed Sep 08 18:15:15 2010 bobtfish [...] bobtfish.net - Status changed from 'open' to 'rejected'
  Wed Sep 08 22:45:55 2010 evan [...] dealermade.com - Correspondence added
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>
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
  Wed Sep 08 22:45:58 2010 The RT System itself - Status changed from 'rejected' to 'open'
  Thu Sep 09 14:45:12 2010 bobtfish [...] bobtfish.net - Correspondence added
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...
  Thu Sep 09 14:45:14 2010 bobtfish [...] bobtfish.net - Status changed from 'open' to 'rejected'
  Thu Sep 09 16:40:39 2010 perl [...] evancarroll.com - Correspondence added
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
  Thu Sep 09 16:40:47 2010 The RT System itself - Status changed from 'rejected' to 'open'