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

Add missing strictures #13

Merged
merged 2 commits into from Sep 4, 2015

Conversation

Projects
None yet
3 participants
@shlomif
Contributor

shlomif commented Sep 4, 2015

CPANTS Kwalitee.

robkinyon added a commit that referenced this pull request Sep 4, 2015

@robkinyon robkinyon merged commit 1cf4d5c into robkinyon:master Sep 4, 2015

@shlomif

This comment has been minimized.

Show comment
Hide comment
@shlomif

shlomif Sep 4, 2015

Contributor

Thanks!

Contributor

shlomif commented Sep 4, 2015

Thanks!

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Sep 8, 2015

@shlomif FATAL => 'all' prliferation is a generally bad idea and is officially discouraged by p5p. Please consider following up with a reversal PR.

ribasushi commented Sep 8, 2015

@shlomif FATAL => 'all' prliferation is a generally bad idea and is officially discouraged by p5p. Please consider following up with a reversal PR.

@shlomif

This comment has been minimized.

Show comment
Hide comment
@shlomif

shlomif Sep 8, 2015

Contributor

@ribasushi : hello! The «FATAL => 'all'» for the «use warnings;» lines exist in all other .pm source files in DBM-Deep, so I added it in the missing .pm as well for consistency. I realise it's a bad idea and avoid using it for my own code. Maybe you should file an issue for it for DBM-Deep.

Contributor

shlomif commented Sep 8, 2015

@ribasushi : hello! The «FATAL => 'all'» for the «use warnings;» lines exist in all other .pm source files in DBM-Deep, so I added it in the missing .pm as well for consistency. I realise it's a bad idea and avoid using it for my own code. Maybe you should file an issue for it for DBM-Deep.

@robkinyon

This comment has been minimized.

Show comment
Hide comment
@robkinyon

robkinyon Sep 8, 2015

Owner

@ribasushi - The official discouragement came years after I stopped actively working on DBM::Deep. I'd be happy to accept a PR that fixes the problem correctly (instead of just removing the FATAL => 'all').

Owner

robkinyon commented Sep 8, 2015

@ribasushi - The official discouragement came years after I stopped actively working on DBM::Deep. I'd be happy to accept a PR that fixes the problem correctly (instead of just removing the FATAL => 'all').

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Sep 8, 2015

@robkinyon my comment wasn't a critique of the module as a whole, just of this PR. Blindly removing FATAL's without a code audit is just as dangerous as adding them where there were none (what this PR did). To put it in terms an engineer would understand:

  • Adding crap to a structure without a containment tank is bad - god knows what it may contaminate
  • Removing crap that has been there for years is equally bad - god knows what lifeform it actively supports

In any case - the original comment was simply made in passing, about something minor-ish I noticed. No action being taken is a perfectly reasonable outcome.

ribasushi commented Sep 8, 2015

@robkinyon my comment wasn't a critique of the module as a whole, just of this PR. Blindly removing FATAL's without a code audit is just as dangerous as adding them where there were none (what this PR did). To put it in terms an engineer would understand:

  • Adding crap to a structure without a containment tank is bad - god knows what it may contaminate
  • Removing crap that has been there for years is equally bad - god knows what lifeform it actively supports

In any case - the original comment was simply made in passing, about something minor-ish I noticed. No action being taken is a perfectly reasonable outcome.

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