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

Enabling warnings on all lib/* perl code files #23

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jluis
Contributor

jluis commented May 28, 2015

This is part of my CPAN Pull request challenge.

After enabling warnings I cleaned compile time warnings using lexical scoped "no warnings qw(once);"
when possible or pairs (no,use) around the offending code.

After that I cleaned the warnings emitted during make test, all where undefined warnings, using a "defined ... and ..." on Boolean expressions or "no warnings qw(undefined);" where appropriate as all of them where on the last exresion of a block

jluis added some commits May 27, 2015

enabling warnings on lib/DBD/
using scoped and specifics  no warnings on full qualified names or the par
no /use  with the specific list

Signed-off-by: Jose Luis Perez Diez <jluis@escomposlinux.org>
enablig warniongs in the resot of the lib/* files
Signed-off-by: Jose Luis Perez Diez <jluis@escomposlinux.org>
Cleaning the warning emitted during make test;
Almost all are using lexical scoped no warnings and
replacing if ( expression) with if (defined $var and expression)

Signed-off-by: Jose Luis Perez Diez <jluis@escomposlinux.org>
@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi May 28, 2015

Member

@timbunce & crew: The proposed change is very intrusive (its merit aside). Potential spurious warnings may break downstream test suites and/or fill up production logs.

I am not advocating the PR is plain rejected, but please weigh it carefully. If the PR is merged please make sure there is a dev release of DBI, which subsequently is smoked for at least ~4 weeks (enough time for @andk's rolling smokers to adequatelly test it).

Member

ribasushi commented May 28, 2015

@timbunce & crew: The proposed change is very intrusive (its merit aside). Potential spurious warnings may break downstream test suites and/or fill up production logs.

I am not advocating the PR is plain rejected, but please weigh it carefully. If the PR is merged please make sure there is a dev release of DBI, which subsequently is smoked for at least ~4 weeks (enough time for @andk's rolling smokers to adequatelly test it).

@wjgeorge

This comment has been minimized.

Show comment
Hide comment
@wjgeorge

wjgeorge Jul 3, 2015

Shouldn't the "defined(..) and " changes be "defined(...) &&"? Using short circuit instead of Boolean.

wjgeorge commented Jul 3, 2015

Shouldn't the "defined(..) and " changes be "defined(...) &&"? Using short circuit instead of Boolean.

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge Jul 3, 2015

Contributor

@wjgeorge the behaviour is the same - the only difference between and and && is their precedence relative to other operators.

Contributor

karenetheridge commented Jul 3, 2015

@wjgeorge the behaviour is the same - the only difference between and and && is their precedence relative to other operators.

@timbunce

This comment has been minimized.

Show comment
Hide comment
@timbunce

timbunce Jul 18, 2015

Member

I've been giving this one some thought and I find myself agreeing with @ribasushi and erring on the side of caution. It seems to me that the significant potential risks outweigh the relatively small gains.

Even with the suggested "dev release of DBI, which subsequently is smoked for at least ~4 weeks (enough time for @andk's rolling smokers to adequatelly test it)" we wouldn't be able to assess the risk of warnings breaking "downstream test suites and/or fill up production logs". And adding checks for zero warnings from our test suite wouldn't help much.

Thank you for your work @jluis. It is greatly appreciated. I'm sorry it didn't work out this time. I'm especially sorry for not considering the pros and cons more deeply before encouraging you in this direction.

I hope you're not put off and that you'll contribute again soon.
Thank you again.
Tim.

Member

timbunce commented Jul 18, 2015

I've been giving this one some thought and I find myself agreeing with @ribasushi and erring on the side of caution. It seems to me that the significant potential risks outweigh the relatively small gains.

Even with the suggested "dev release of DBI, which subsequently is smoked for at least ~4 weeks (enough time for @andk's rolling smokers to adequatelly test it)" we wouldn't be able to assess the risk of warnings breaking "downstream test suites and/or fill up production logs". And adding checks for zero warnings from our test suite wouldn't help much.

Thank you for your work @jluis. It is greatly appreciated. I'm sorry it didn't work out this time. I'm especially sorry for not considering the pros and cons more deeply before encouraging you in this direction.

I hope you're not put off and that you'll contribute again soon.
Thank you again.
Tim.

@timbunce timbunce closed this Jul 18, 2015

@jluis

This comment has been minimized.

Show comment
Hide comment
@jluis

jluis Jul 20, 2015

Contributor

@timbunce no problem, I wasn't sure miself mainly becase there are lots of modules depending on DBI in CPAN

Thaks,
jluis

Contributor

jluis commented Jul 20, 2015

@timbunce no problem, I wasn't sure miself mainly becase there are lots of modules depending on DBI in CPAN

Thaks,
jluis

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge Jul 20, 2015

Contributor

I would at least merge the no warnings '...' snippets, as they mark places where we know warnings occur.

It would also be reasonable to add use warnings; in smaller scopes (e.g. individual functions) as code inspection determines that that area is safe and warnings-free.

Contributor

karenetheridge commented Jul 20, 2015

I would at least merge the no warnings '...' snippets, as they mark places where we know warnings occur.

It would also be reasonable to add use warnings; in smaller scopes (e.g. individual functions) as code inspection determines that that area is safe and warnings-free.

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