On Fri, Apr 9, 2010 at 8:51 AM, Evan Carroll <[email protected]> wrote: >> Without any unnecessary commentary, here is the implementation of the >> password_(pre|post)_salt_field, without other features that should be >> patched separately. >> >> http://codepeek.com/paste/4bbf456c0ae3049443a742a2 > > I appreciate your efforts. > > Without any unnecessary commentary (yes, it is all necessary) you > can't pry code from my moosified version out, without reading what > Moose was offering (namely, the methods you're using like get_config > -- I take it you liked it). Your version is a slight disservice as > you're ignoring the test patches and docs patches. So what I have 4 > patches -- completely lacking negative criticism might I add -- they > also put the dev team in the direction they want to go with Moose. >
Please provide a link to the tests, as I didn't see any modifications in the diffs. get_config could be easily applied. I wouldn't commit this until there are adequate tests, anyway. My specific problems with your patch is: 1) You arbitrarily change the style. It's difficult to find functional changes when every line in the diff is changed so you can change the way spaces are. You did this same thing with Catalyst::View::Email. If you want to propose stylistic changes, do that in a separate patch from functional changes. 2) Adding the salt from the database field doesn't require Mooseification. It does require tests. I didn't see tests. 3) There isn't a problem with going towards Moose. There is, however, a problem with doing 3 different things at once and just saying, "It's a point release". This is due diligence, nothing more. I don't like to see large changes to any code I use. I've already had several points of breakage with DBIC (one severe) that are holding me back to previous versions (and still no tuits to fully chase it down beyond a ML post) Authentication is a big deal, and an important component, I want to see good patches go in but care needs to be taken. Understand that your method of submitting patches puts a huge burden on those accepting and integrating those patches. Your brazen attitude makes you easy to ignore, even if the point you raise is valid. _______________________________________________ List: [email protected] Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/[email protected]/ Dev site: http://dev.catalyst.perl.org/
