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 #46

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@fluca1978
Contributor

fluca1978 commented May 4, 2015

This is a tiny set of commits to improve the already high quality module.
I took a look at a couple of open issues and tried to find out a good solution, as well as doing some minor changes to the style of some small pieces of code in order to improve readability.
Hope this can help.

@tm604

This comment has been minimized.

Show comment
Hide comment
@tm604

tm604 May 24, 2015

Just one minor point: I think 86d0921 should be reverted, or at least raised separately. Most of the affected methods look like they return scalar values, so replacing 'return undef' with 'return;' will break code such as this:

somefunc($x->find_method_by_name('some_missing_name'), 123)

since it'll end up calling somefunc(123) if the 'some_missing_name' method isn't found.

Also, if this didn't cause test failures then I'd suggest it's worth adding some tests to verify that those methods still return undef in list context.

tm604 commented May 24, 2015

Just one minor point: I think 86d0921 should be reverted, or at least raised separately. Most of the affected methods look like they return scalar values, so replacing 'return undef' with 'return;' will break code such as this:

somefunc($x->find_method_by_name('some_missing_name'), 123)

since it'll end up calling somefunc(123) if the 'some_missing_name' method isn't found.

Also, if this didn't cause test failures then I'd suggest it's worth adding some tests to verify that those methods still return undef in list context.

@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 May 25, 2015

Contributor

While I believe it can be cherry-picked, I've created another branch without the offending commit about the return undef.

Contributor

fluca1978 commented May 25, 2015

While I believe it can be cherry-picked, I've created another branch without the offending commit about the return undef.

@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 May 25, 2015

Contributor

See here #48

Contributor

fluca1978 commented May 25, 2015

See here #48

@fluca1978 fluca1978 closed this May 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment