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

Mouse won't warn when overwriting a method with an accessor #86

Open
ybrliiu opened this issue Mar 5, 2018 · 5 comments
Open

Mouse won't warn when overwriting a method with an accessor #86

ybrliiu opened this issue Mar 5, 2018 · 5 comments

Comments

@ybrliiu
Copy link
Contributor

ybrliiu commented Mar 5, 2018

In Moose

package Foo {
  use Moose;
  has hello => ( is => 'ro' );
  sub hello { }
}

=> You are overwriting a locally defined method (hello) with an accessor

In Mouse

package Foo {
  use Mouse;
  has hello => ( is => 'ro' );
  sub hello { }
}

=> No warning and error

I guess Mouse should warn in such case.

@ybrliiu
Copy link
Contributor Author

ybrliiu commented Mar 5, 2018

Warning will be issued if code in comments restored (lib/Mouse/Meta/Attribute.pm line 260 ~ 264).
But we must do something for compatibility with Moose (lib/Mouse/Meta/Attribute.pm line 259).

@ybrliiu
Copy link
Contributor Author

ybrliiu commented Mar 5, 2018

Probably we must pass the tests in t/020_attributes/027_accessor_override_method.t?

@ybrliiu
Copy link
Contributor Author

ybrliiu commented Mar 31, 2018

メソッドをアクセッサでオーバーライドしたとき警告を出す処理の部分(lib/Mouse/Meta/Attribute.pm の 260 ~ 264行目)のコメントアウトを解除し、 t/020_attributes/027_accessor_override_method.t のテストをスキップせずすべて実行したところ、passしてました。
もし問題なければ警告を出す部分のコメントアウトを解除して警告がでるようにしたいのですが、その上の部分のコメント(we must do something for compatibility with Moose)というのが気になっていまして・・・
Mooseとの互換性のために他に何か実装しなければならないことがあったりするのでしょうか?

@skaji skaji closed this as completed Aug 12, 2018
skaji added a commit that referenced this issue Aug 13, 2018
Changelog diff is:

diff --git a/Changes b/Changes
index 7e69336..808e88e 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,8 @@
 Revision history for Mouse

 {{$NEXT}}
+
+v2.5.5 2018-08-13T15:41:32Z
     - Warn if accessors overwrite methods/functions (ybrliiu #86, #90, #93)
     - Fix for threads and XS; use newSVpvs instead of newSVpvs_share (sergeykolychev #92)
skaji added a commit that referenced this issue Aug 13, 2018
Changelog diff is:

diff --git a/Changes b/Changes
index 808e88e..f31e5e0 100644
--- a/Changes
+++ b/Changes
@@ -2,6 +2,9 @@ Revision history for Mouse

 {{$NEXT}}

+v2.5.6 2018-08-13T22:47:57Z
+    - Revert "Warn if accessors overwrite methods/functions" for now; it may cause crashes in perl 5.28.0 (#94)
+
 v2.5.5 2018-08-13T15:41:32Z
     - Warn if accessors overwrite methods/functions (ybrliiu #86, #90, #93)
     - Fix for threads and XS; use newSVpvs instead of newSVpvs_share (sergeykolychev #92)
@skaji skaji reopened this Aug 13, 2018
@skaji
Copy link
Member

skaji commented Aug 13, 2018

@ybrliiu

use strict;
use warnings;

{
    package Foo::Role;
    use Mouse::Role;

    has 'bar' => (
        is      => 'rw',
        isa     => 'Int',
        default => sub { 10 },
    );

    package Foo;
    use Mouse;

    with 'Foo::Role';
    has '+bar' => (default => sub { 100 });
}

This code is valid and should not emit redefine warnings, so I've merged PR #93.

On the other hand, it may cause crashes in perl 5.28.0 :/
So I reverted the whole change in #94 for now.

I will consider how to fix/workaround this.
Thanks for your patience.

@ybrliiu
Copy link
Contributor Author

ybrliiu commented Aug 18, 2018

I see.
Thanks for your follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants