Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPAN Pull Request Challenge 2015 #1

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
2 participants
@fluca1978
Copy link

commented Feb 18, 2015

Dear brian,
this is a small attempt at improving the excellent code of your module.
It is mainly made by small cosmetic changes to improve readability and, to some extent, documentation.
Hope it helps.

fluca1978 added some commits Feb 16, 2015

Added comments to method that require optionals modules.
Both the count_lines and file_magic methods require optional modules,
respectively SourceCode::LineCounter::Perl and File::MMagic.
Both the method try to load the modules via eval, and if the modules
are not found, the method return an undef value. Let's document it.
Remove a TODO item that was referencing already existing methods.
The count_lines and file stats are already implemented, so there is no
need to list them as todo anymore.
Remove dead code in the main class cleanup implementation.
Added a comment with an example on what the cleanup method should do if
the removal of a whole path tree is required.
Add variable to store the same message used in run_info and error
reporting.

Making the two messages always coherent.
Remove fixed path.
Use File::Spec and File::HomeDir to point to the
user home directory for the tied hash database.
Since File::HomeDir->my_desktop could return undef if the desktop is not
available, the method defaults to the user's home directory if the
desktop is not found.

Added dependency to Makefile.PL, using version 0.07 as lower since it is
the one with the older API interface.
Refactor if-else branch.
The eval exception ($at) is already included in the error log, and all
the cases were returning (stopping) setting different run_info depending
on the fatality-ness of the exception. Therefore the triple if-elsif
branch can be compressed in a single branch with the discrimination on
the run_info information and a single log/error statement that informs
about the "stopping" of the execution.
@briandfoy

This comment has been minimized.

Copy link
Owner

commented Mar 9, 2015

There are a few things in this pull request that I should fix and I'll look at these things, but at the same time you've included lots of unrelated changes that don't really improve anything or are matters of style that make it hard to cherrypick the good stuff. The better way to handle this is to check if a such changes would be welcome then send smaller pull requests that contain only the related fixes. As such, I'm closing this request but I'll enter some of the changes by hand.

Thanks,

@briandfoy briandfoy closed this Mar 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.