Reviews by Dan Muey



HTML-Truncate (0.16) *****

Nice job, thanks!

HTML-Obliterate (0.3)

> This is a terrible implementation of a bad idea. Regular expressions for HTML parsing
> are extremely difficult to get right.

It's not meant to be a comprehensive parser neither does it claim to be such. All it does is "removes all HTML tags and entities".

> This module will erase content including entities, instead of converting them which is
> trivial which HTML::Entities, and markup like "1 < 2;" while bad, plenty of HTML in the
> wild contains stuff like this.

The only claim it makes is "removes all HTML tags and entities".

If you want to preserve certain entities you should use HTML::Entities to decode (and therefore keep) the ones you want. (I'll add that to the POD in the next version) Also to strip tags and what it contains <script>code code code</script> you'd need a more indepth HTML::Parser based module. (I'll also add that to the POD)

> It is trivial to strip HTML yourself with HTML::TokeParser or HTML::TokeParser::Simple or > XML::LibXML or ...

HTML::Strip is also available for a parse-into-a-tree-and-clean-up-the-tree approach.

Again though, this is not mean to be a convert-HTML-into-equivalent-text module but a quick "no HTML please" just as it says so I'm pretty shocked at the intensity of your hatred.

> The code makes it look like it should have been in the Acme namespace. 12 subs all
> synonyms for calling, via goto, the real routine.

So no one is allowed to have fun with their code?

> Do *not* put this module into production code.

Sure don't use it if it doesn't fill your need, use it if it does.

> This is the sort of code that gives Perl and the CPAN a bad name.

Haters like you give CPAN a bad name. Relax, have fun!

Hash-Merge-Simple (0.03) *

- Already have Hash::Merge that does this and more if you need

- no way to set precedence rules or how to handle value types

- different results that what the standard Hash::Merge users are used to so it can;t be dropped in as a replacement

- "doesn't attempt to combine arrays, objects, scalars, or anything else"

no way to specifcy the behavior you want = more complexity of re-recursively checking your data again.

again, different results than Hash::Merge

- "The code does not currently check for cycles, so infinite loops are possible"

So you have to recursively sanitize the data that you're passing to be recursively merged or else you could bring down the process or even th eentire machine?

Its more "Complex" than "Simple"

---- in Replay to Jonathon's review ---

> This is a nice way to merge hashes. It has a few advantages over Dan's Hash::Merge.
> Firstly, it wasn't written by him.

Really? Seriously? Come on what is your problem? This is very unhelpful. Your reviews are seriously lacking. If I've dome something wrong let me know and I'll fix it, why be a jerk?

FYI, Hash::Merge wasn't written by me either, I just offered to be co maintainer so I could fix a bug and tidy up the code a bit.

> Secondly, the code in question has proven itself in thousands of production Catalyst applications.

*This* is helpful, good job.

> Finally, sometimes ::Simple is the way to do things. I don't want to devote thought to
> merging hashes, I just want it to happen

I tend to agree, however as I pointed out above its not so simple, there's a lot of other things you have to consider or you might end up in trouble.

DBI-Mysqlsimple (0.02) *

Should be in DBIx

Mysqlsimple is also a very bad name space

re-implements do() and disconnect() (chance for behavioral changes)

makes it more complicated to do use dbh object methods

no way to make queries efficent by:

- reusing statement handles

- going though multiple records one at a time instead of slurping them all into memory first

Also a point of complexity and confusion:

get_row (singular) returns an array (possibly plural)

get_rows (plural) returns one item (singular)

it makes sense to return that type of data for tha ttype of requestt and the method names make sense (as long as you speak english) but its not something intuitive, you have to consult the docs every time you want to use this.

So its simple in the fact that it has a few basic shortcut methods (that are already in DBI) but its actually more complex for the reasons outlined above.

Acme-Tiny (0.3)

[for 0.3] once the PDO was there:

Thank you David, at least Mr. Cantrell gets it.

The rest of the neigh-sayers might want to re-read the 'DESCRIPTION' and the sub headings under it, namely:

'This module is stupid'

'What it might be useful for...'

'I still think its stupid'

'Why don't you spend time on useful stuff instead of this kind of junk?'

[for 0.1] hopefully you might have caught:

Whoops, forgot the .pod file in the manifest, its uploading now, My bad.

Please, wait until you get the POD to comment or vote please. The POD explains everything...

[Jonathan Rockway]

You complained about Acme::Tiny and bashed me, even insulted me for it but:

- Nothing::Tiny isn't even ::Tiny, version '1' uses warnings and our().

- you complain about my POD but at least it has benchmark results, ::Tiny information, etc

- You didn't use Acme:: for a goof off module

Perhaps if you read the detailed POD you might learn something. Also your comments are neither accurate nor useful. They also serve to discourage people shopping for languages from using perl, nice work.

[Earle Martin]

Did I do something to offend you or something, whats with all the road rage?

lib-tiny (0.6)

[UPDATE] the "compiled" part was removed after 0.4 since it wasn't "::Tiny"[/UPDATE]

Adam is correct, its intent is memory, though it does save CPU time as well.

The 'compiled' part is optional though (its a variable substitution in Makefile.PL to avoid having to use Config).

The idea is that if someone is 'dropping it in' they probably aren't worried about having the archname/version directory structure checked also.

That being the case it may be better (IE more '::Tiny') if it just avoided it all together and just added it to @INC if its not there already. Thanks for pointing that out Adam. Your time and feedback are appreciated!

> WOOOOO!!! SHAVE A WHOLE 0.005s! TEH OSOM!!!11one

Where exactly did I say it has to be used everywhere by everyone?

There are some environments where any memory gain (and CPU gain for that matter) are critical.

Also, your sample output compares use lib vs. not doing use lib so the "test" doesn't even make sense, you'd need to compare the tiny version to the un-tiny version:

time perl -e 'use lib "/tmp";'

time perl -e 'use lib::tiny "/tmp";'

but again CPU wasn't the primary goal anyway, just a happy side effect.

As side note, its people like you doing pointless personal rants like this that turn folks away from perl which is very sad.

If there is a deficiency in a module I do I want to fix it. If I see a deficiency in someone else's module I often offer to help with it or at least offer a suggestion.

but a personal attack like this (based on obviously flawed testing logic) is not only pointless it reflects bad on you.

Class-Std (0.0.9) *****

I'm very grateful for this excellent module, the associated PBP is excellent.

We're also happy to announce that 0.0.9 marks a great step toward very active development.

In fact there is a section in the POD called:

'I know we're all busy, I'd like to help maintain Class::Std!'

that shows how you, yes you, can 'do something about the weather' instead of just moping that its not being maintained or you're being ignored :)

As for the many rt reports, I'm in the process of sorting them out and found over half so far were already addressed by Damian. So its not as bad as the neigh sayers say!

After I sort through the remaining reports I suspect a new release including many of the simpler features/fixes and then we can all focus on the more pedantic ones.


Digest-MD5-File (0.06)

Thanks for your input. I've commented inline to your comments, I hope the response helps get better info and a higher rating :)

> Major Problems:
> No meaningful tests.

0.06 added a couple, more would be great!

> 'file_md5', 'url_md5', etc. reads entire files into memory rather
> than using Digest::MD5's addfile() function or reading the file in
> chunks and using Digest::MD5's add(). (Strangely, however,
> addpath() does not have this problem.)

Addressed in 0.06 thanks to a good rt report. URLs of course are slurped in still (ala LWP), patches to improve that are welcome.

> 'adddir' relies on the order of keys in a hash to produce
> consistent results, which is broken since 'identical' directories
> may be read in different orders and because of hash randomization.

Where does it relay on the order of keys? It works as outlined:

"Returns a hashref whose keys are files relative to the given path and the values are the MD5 sum of the file or and empty string if a directory. It recurses through the entire depth of the directory. Symlinks to files are just addpath()d and symlinks to directories are followed."

order is never mentioned.

Perhaps an enhancement can be made if you can be clearer on the problem you experienced. Code is required.

> Passing binmode argument to 'file_md5*' does not actually do
> anything.

It opens any filehandles in binmode, can you actually substantiate that accusation with some code or are you just confused as to it purpose? If it does not work as advertised we can fix it ASAP.

> Minor Problems:

these are good also, I have a suggestion though:

Instead of a mostly pointless rant that an author will probably never see (IE not alerted to to new posts) why not open an rt report so these things can be addressed (IE alerted with new or updated tickets)?

Data-Rand (v0.0.2)

Let me start by saying thanks for your feedback. While I do appreciate it it'd be more useful to offer ideas for improvement instead of mostly just rants. I hope the ideas presented below will help improve it and get you to rate it higher in version 0.0.3

For future reference what I try to do when I comment on a module that I feel could be improved is address each item in a respectful way and offer a patch if that makes sense or at least a specific suggestion. For example:

"The code is crap, its not strict or warnings safe and is hard to read."

while that may be 100% true this is a more prodcutive way:

"The code format is difficult to follow since there is no whitespace an dthe variable names are all ambiguous and is not strict or warnings safe. I'd recommend adding use strict; and use warnings; and apply some perl best practice principles to it. Attached is a version I ran through perltidy with these settings: .... I also added strict and warnings but that breaks test 'xyz' and 'def' so that issue will still need addressed.

Can you see how much more productive that way is?

> Definitely *not* cryptographically secure.

Care to share what specifically that statement is based on and how it might be corrected?

My statement is based on info in perldoc, specifically this (and the part quoted further down):

perdoc -f srand:

Most programs won't even call srand() at all, except those that

need a cryptographically-strong starting point rather than the

generally acceptable default, which is based on time of day,

process ID, and memory allocation, or the /dev/urandom device,

if available.

I'll qualify that by changing 'cryptographically strong' to 'cryptographically-strong starting point' to be more clear. and otherwise make that point clearer where possible. Thanks.

I did just now notice that I forgot to add the tests for the 3 functions to the MANIFEST so they are not included. They include 37 tests to how it functions with different args as per the pod and 10,000 tests for actual randomness reliability. Perhaps that will help address your concerns as well.

> Uses one call to rand() (typically 15 bits of "randomness" but even that's questionable), the process id and the time of day to seed an array,

no, its to seed rand() ( IE srand() ) and has nothing to do with any arrays...

Again from perdoc -f srand :

Note that you need something much more random than the default

seed for cryptographic purposes. Checksumming the compressed

output of one or more rapidly changing operating system status

programs is the usual method. For example:

srand (time ^ $$ ^ unpack "%L*", `ps axww | gzip`);

If you're particularly concerned with this, see the "Math::Tru-

lyRandom" module in CPAN.

Which is why the POD says:

A few examples in line with srand perldoc:

sub { return unpack '%L*', `ps axww | gzip` }


Additionally, this is what 'SEEDING RANDOM GENERATOR' describes and the the 'srand_seeder_coderef' and 'use_time_hires' keys are for.

Again, more indication of the need to RTFM and do it as per your needs will be included so as to avoid misconcetpion and ambiguity.

> and then shuffles that around to generate chars.

It does not shuffle the srand args it shuffles the list of items to use tobuild the random data out of. This adds a number of rand() calls (see shuffle() source) and adds to its effectiveness because not only do they have to figure out that the randomdate was indexes 9 2 and 5 the data at said indexes will be different as well.

> Not efficient, either.

What benchmark results are you basing that comment on? Please elaborate so I can benchmark improvements. If its a guess or opinion leave them out of comments or at least qualify the statements.

> If you want real random data read it from /dev/random, use an openssl binding, use the entropy gathering daemon, or something like that.

Exactly the point of the statement 'If a random int between 0 and 999,999,999,999,999 is not what you want for that part of the calulation, feel free to change it via the hashref argument described above.' If you want to privced coderef example of a 'real random data' producing srand_seeder_coderef I'd be happy to review and add to the docs as examples.

> Cryptographically secure algorithms

This is not an algorithm, neither does it claim to be such.

> should be designed by cryptographers, and much more importantly rigorously reviewed by cryptographers before making such claims.

Feedback is welcome, I'm happy to improve it with clearer info / better code but so far your feedback is very unhelpful.

cPanel (v0.0.1)


This module and the name space was propery registered for a set of corporate modules under a proper namespace and proper procedure.

I understand you do not like a few things I have done but please stop commenting on things until you have the full story.

If you have a problem with something I have done please contact me off list and I'd be happy to rectify any mistakes.

last (v0.0.1)

You don't get it do you?

DBIx-Std (v0.0.1)

Sorry guys, I've been very busy and didn;t plan on it taking this long. Once its done i think you'll love it, no need to bash it until you try it.

Perhaps you have all sorts of extra time but some of need to make a living so please don't berate something you don't have a clue about, thanks. I'd rather put out a complete product then a partial one but thats just me, I prefer well-thought-out quality.

Perhaps wait until its out to try it and then rate its actual functionality, interface, etc.

but if that still upsets you then feel free not to use it, either way I do not apologize for being too busy to work for you for free

try to be more understanding, we're all in this together!

base-ball (v0.0.2)

I'm disappointed these two reviews gave a one. It seems they misunderstood the intention of the module. (0.0.3 adds some 'don't do god classes or misuse ISA' and 'only use it when' in the POD for good measure)

But still I'd like to address the misconceptions and misunderstandings.

Its not trygin to enforce any inheritance practice, its just a small tool that does what it does if that whats you need. The POD is very descriptive of that. For someone to decide they need this module they;d have already had to have created their inheritance structure.

The *only* thing it does is recursively use base based on the arguments you give it as per the POD.

Any way here goes:

> This module is another example of why Perl 5 OO has such a bad name.

ok, so you don't like L<base> either then? Thats all its doing

> Inheritence and IS-A relationships should not be used to
> import tons of methods into a single namespace

All it does is use base as per its arguments. All its doing is calling use base for you a few times. A shortcut for what you'd have to code normally.
Its up to the coder to not misuse inheritance.

> ... This module should *not* be in the base:: namespace,

In fact it should be in the base name space because its a shortcut to use of base. Nothing more nothing less

> because honestly it has *nothing* to do inheritence.

But you just implied it was misusing inheritance and now it has nothing to do with it? All it "has to do with" is described in 'DESCRIPTION'.

(and 0.02 doesn't even say 'inheritance', IS-A, or ISA)

> And this is just the tip of the iceberg (yes, I think this module has many hidden dangers
> along with its total warping of OO theory) see the RT bug queue for a good discussion
> of these dangers.

This concern really has nothing to do with this module but inheritance theory in general. All this does is use base a few times for you based on your args as per the POD. So it only makes god classes if the coder is making a god class and misuses @ISA if the coder is misusing @ISA. Of course they can do the exact same thing by calling use base several times to accomplish the same thing.

If you read the DESCRIPTION of what it does its very very simple. Can it be misused? Sure, but so can use base, so can a car, so can a knife, so can fire...

Also the rt you pointed out ( L<; ) has some excellent info of what not to use this module for and why it is or is not some of the items pointed out.

> Another unnecessarily silly module: The name lacks any clue about what the module
> actually does, saying "ball" rather than "all".

It was meant to be a (silly) linguistic play on words and slight reference to America's favorite pasttime: Perl!.

Technically base::recursive would be more accurate. Sorry if the joke was confusing, I was hoping the NAME part would explain the joke.

Its been very handy and worked very well for what its intended for and since it was intended to be rarely used I figured a little humor would be ok.

> It's also rather too magic to actually use, since adding any module suddenly
> means your code is inheriting from it.

But you'd only add a module if you wanted to inherit from it. Whats so magical about that? Its as complex and magical as:

use base 'Foo::Utils';

use base 'Foo::Utils::Sys';

use base 'Foo::Utils::Etc';

> Module::Pluggable is much more practical.

It may suit your needs better true, but its an entirely different idea. Have a gander: L<Module::Pluggable> and feel free to use it. Its a great module.

Also how is Module::Pluggable not 'too magical to actually use' if base::ball is? Have you actually *looked* at either? Do you actually understand what either is doing?

I also recommend L<Moose> for even sexxier Perl 6 style classing/objects. Of course its veeeery magical and "The name lacks any clue about what the module actually does", (all of which is not bad)