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

Fix #406 deprecated code #408

Merged
merged 6 commits into from
May 16, 2014
Merged

Fix #406 deprecated code #408

merged 6 commits into from
May 16, 2014

Conversation

hollie
Copy link
Owner

@hollie hollie commented May 11, 2014

Fixes the deprecated warnings according to issue #406.

Note: I only fixed the code under 'lib' and not in '/lib/site/' as those modules are CPAN modules. Can't we just get recent versions from those from CPAN when deploying MisterHouse instead of keeping track of those in our own codebase?

@@ -275,7 +275,7 @@ sub default_hop_count
if (defined($hop_count)){
::print_log("[Insteon::BaseObject] DEBUG3: Adding hop count of " . $hop_count . " to hop_array of "
. $self->get_object_name) if $self->debuglevel(3, 'insteon');
if (!defined(@{$$self{hop_array}})) {
if (defined $$self->{hop_array} && !(@{$$self{hop_array}})) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ($$self{hop_array} && !(@{$$self{hop_array}})) {

works for me. Please confirm that the test performs the same function as the original line.

@hollie
Copy link
Owner Author

hollie commented May 12, 2014

Hey @JaredF I have added your patch to the pull request. Thanks. According to me this code still performs what the original author intended.

@@ -275,7 +275,7 @@ sub default_hop_count
if (defined($hop_count)){
::print_log("[Insteon::BaseObject] DEBUG3: Adding hop count of " . $hop_count . " to hop_array of "
. $self->get_object_name) if $self->debuglevel(3, 'insteon');
if (!defined(@{$$self{hop_array}})) {
if ($$self{hop_array} && !(@{$$self{hop_array}})) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite right. The logic here should be if hop_array does not exist do something. The proposed code would skip the something if the array does not exist and the hash value has not been declared.

The line should just be if (!(@{$$self{hop_array}})) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, no that won't work with strict enabled. But this does if (!$$self{hop_array} || !(@{$$self{hop_array}})) {

So now if the hash key is undefined OR the value is not an array then we do something. That should do it.

See #411 for my pull request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this makes sense. Didn't realize that it was strict that was throwing the error. @hollie's original pull was if (!(@{$$self{hop_array}})) { but we had changed it since it was throwing the error from strict.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Kevin,

On Thu, May 15, 2014 at 10:10:01AM -0700, Kevin Robert Keegan wrote:

@@ -275,7 +275,7 @@ sub default_hop_count
if (defined($hop_count)){
::print_log("[Insteon::BaseObject] DEBUG3: Adding hop count of " . $hop_count . " to hop_array of "
. $self->get_object_name) if $self->debuglevel(3, 'insteon');

  •   if (!defined(@{$$self{hop_array}})) {
    
  •   if ($$self{hop_array} && !(@{$$self{hop_array}})) {
    

Oops, no that won't work with strict enabled. But this does if (!$$self{hop_array} || !(@{$$self{hop_array}})) {

So now if the hash key is undefined OR the value is not an array then we do something. That should do it.

I have no idea what the above test is meant to accomplish but I have a
few comments, just in case they help to find issues with the new code:

  • The original test apparently wanted to check if $self->{hop_array} (an
    array reference) existed.
  • I think the new test before Kevin's comment checks that
    $self->{hop_array} is not undef, and that the size of the array is zero
    (using an array in scalar context produces the length of the array). Is
    this not correct?
  • Kevin's proposed change actually changes the logic result of the test.
    Perhaps the original test as proposed was incorrect and Kevin's proposed
    change fixes it, I don't know. But I do now that A AND B = (NOT A) OR
    (NOT B) so if the original test is:

$$self{hop_array} && !(@{$$self{hop_array}})

it can be re-written as follows without changing the logic result:

!$$self{hop_array} || (@{$$self{hop_array}})

But Kevin's proposed change leaves the NOT ('!') in the operand after
the logical OR. Is that intended?

Cheers,

Eloy Paris.-

@hollie
Copy link
Owner Author

hollie commented May 15, 2014

Hey @JaredF,

can you please run a final test on this branch so that we can merge it into master if it works fine for you?

Thanks!
Lieven.

@hollie hollie added this to the Next stable 3.2 milestone May 15, 2014
@JaredF
Copy link
Collaborator

JaredF commented May 15, 2014

No problem; will test it tonight.

@JaredF
Copy link
Collaborator

JaredF commented May 16, 2014

Just tested it and everything looks good. No more deprecated code warnings and MH starts up successfully.

hollie added a commit that referenced this pull request May 16, 2014
@hollie hollie merged commit 129f4f4 into master May 16, 2014
@hollie hollie deleted the fix_#406_deprecated_code branch January 8, 2017 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants