[EMAIL PROTECTED] via RT wrote:
--- El mar 30-sep-08, Christoph Otto <[EMAIL PROTECTED]> escribió:
De:: Christoph Otto <[EMAIL PROTECTED]>
Asunto: Re: [perl #44457] [TODO] make sure files match test files for DYNPMCs
and DYNOPs etc
A: "Igor ;" <[EMAIL PROTECTED]>
Cc: [EMAIL PROTECTED]
Fecha: martes, 30 septiembre, 2008, 5:49 am
Igor wrote:
<snip>
Hi Christoph,
I send you the patch attached.
Sincerely,
Igor
Hi Igor,
Thanks again for taking the time to contribute.
Here are some pointers:
First, you're trying too hard. I'm sorry to tell you that after
you've spent
so much effort, but this change should be fairly minimal. I'm sure
you've
noticed that there are two tests in t/distro/test_file_coverage.t that almost
do what you need done. Try copy/pasting code from those tests in the PMC
block and modifying it to check for the files you're interested in.
Don't
worry about the PMC: label. You can give it a more appropriate name once the
rest of the test works.
A good rule is to try to use as little code as possible to get the job done..
This doesn't mean that you should use weird one-liners that nobody can
understand, but that you should strive to write simple, easily understood
code. Line 88 in the file is a fairly good example of what *not* to do,
although it'd be ok with a brief explanatory comment.
Also, don't worry too much about generalizing beyond what's needed to
get the
current job done. If the code starts to look ugly, it can be refactored later.
I hope that sets you in the right direction. Feel free to email me if you
have any questions.
Christoph
Hi Christoph,
I forgot attach the file.
Sincerely,
Igor
Hi Igor,
Patch naming depends on personal preferences, but patches are easier to review
when their filenames end in either .patch or .diff. Naming different versions
of a patch with successive numbers (i.e. too_many_frobs_1.patch,
too_many_frobs_2.patch, etc) makes it easier to see which patch is the most
recent when using RT, but this isn't critical.
Also, you only need to reply to [EMAIL PROTECTED] I'm on the
list and will see your message.
This is a much better patch. It was a good idea to refactor the find commands
into a separate sub. The code is cleaner and a little bit more obvious, which
is a good thing.
My only concern is that it looks like you deleted the tests that matches
non-dynamic PMCs to their respective tests. Those tests did fail but they
should be kept in test_file_coverage.t TODO'd, so they can eventually be
addressed. I may end up doing this myself, so be sure to check if the tests
actually need to be TODO'd before sending the updated patch.
Apart from that change, the patch looks good. If you can make those changes
and resubmit, I'll be glad to apply it.
Thanks,
Christoph
