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

Clean inheritance #12

Merged
merged 3 commits into from Apr 10, 2015

Conversation

Projects
None yet
2 participants
@dolmen
Contributor

dolmen commented Apr 9, 2015

Use Exporter and DynaLoader without inheriting from them.
Because they pollute the namespace of our class.
Because an empty @ISA makes methode lookup faster.

dolmen added some commits Apr 8, 2015

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Apr 10, 2015

Collaborator

Looks good thanks. Always wondered why it was the way it was...

Collaborator

smith153 commented Apr 10, 2015

Looks good thanks. Always wondered why it was the way it was...

smith153 added a commit that referenced this pull request Apr 10, 2015

Merge pull request #12 from dolmen/clean-inheritance
Clean inheritance of Exporter and DynaLoader

@smith153 smith153 merged commit 39c956a into Dual-Life:master Apr 10, 2015

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Apr 10, 2015

Collaborator

We might have broken subclassing/inheritance somehow. Module::New fails to build and test with Time::Piece-1.29.04:

t/00_load.t ......... 1/50
Failed test 'use Module::New::Date;'
at /home/user/perl5/lib/perl5/Test/UseAllModules.pm line 71.
Tried to use 'Module::New::Date'.
Error: "gmtime" is not exported by the Module::New::Date module
"localtime" is not exported by the Module::New::Date module
Can't continue after import errors at /home/user/perl5/lib/perl5/x86_64-linux/Time/Piece.pm line 141.
BEGIN failed--compilation aborted at /home/user/perl5/lib/perl5/Test/UseAllModules.pm line 71.
Bailout called. Further testing stopped: failed: Module::New::Date
FAILED--Further testing stopped: failed: Module::New::Date

Haven't looked into it yet, but Module::New builds fine with 1.29

Collaborator

smith153 commented Apr 10, 2015

We might have broken subclassing/inheritance somehow. Module::New fails to build and test with Time::Piece-1.29.04:

t/00_load.t ......... 1/50
Failed test 'use Module::New::Date;'
at /home/user/perl5/lib/perl5/Test/UseAllModules.pm line 71.
Tried to use 'Module::New::Date'.
Error: "gmtime" is not exported by the Module::New::Date module
"localtime" is not exported by the Module::New::Date module
Can't continue after import errors at /home/user/perl5/lib/perl5/x86_64-linux/Time/Piece.pm line 141.
BEGIN failed--compilation aborted at /home/user/perl5/lib/perl5/Test/UseAllModules.pm line 71.
Bailout called. Further testing stopped: failed: Module::New::Date
FAILED--Further testing stopped: failed: Module::New::Date

Haven't looked into it yet, but Module::New builds fine with 1.29

@dolmen

This comment has been minimized.

Show comment
Hide comment
@dolmen

dolmen Apr 11, 2015

Contributor

I'll investigate. On first quick look, I think I'll propose a patch to Module::New. I'll meet its maintainer in the next week at the Perl QA Hackathon.

Contributor

dolmen commented Apr 11, 2015

I'll investigate. On first quick look, I think I'll propose a patch to Module::New. I'll meet its maintainer in the next week at the Perl QA Hackathon.

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Apr 12, 2015

Collaborator

Yea, it could go either way. Wasn't sure which you would prefer.

Collaborator

smith153 commented Apr 12, 2015

Yea, it could go either way. Wasn't sure which you would prefer.

dolmen added a commit to dolmen/p5-Module-New that referenced this pull request Apr 13, 2015

Fix usage of Time::Piece in Module::New::Date
Time::Piece must be either imported or inherited from. Not both. But
this was the case here with 'use base qw( Time::Piece );' as base.pm
calls the import method of the module.
So we now just use Time::Piece as a class to build the value to fit the
Module::New::Context contract.
Note that the previous style may be broken by the next release of
Time::Piece. See Dual-Life/Time-Piece#12

dolmen added a commit to dolmen/p5-Module-New that referenced this pull request Apr 13, 2015

Fix usage of Time::Piece in Module::New::Date
Just use Time::Piece as a class to build the value to fit the
Module::New::Context contract.
Note that the previous style may be broken by the next release of
Time::Piece. See Dual-Life/Time-Piece#12
@dolmen

This comment has been minimized.

Show comment
Hide comment
@dolmen

dolmen Apr 13, 2015

Contributor

I think that we will have to restore inheritance from Exporter, as I see the way in which Module::New::Date use Time::Piece shows me some valid (but tricky) use cases that are now broken. I still have to find if they are really in the wild.

About Module::New::Date, I still think it will be much cleaner with my charsbar/module-new#2 fix.

Contributor

dolmen commented Apr 13, 2015

I think that we will have to restore inheritance from Exporter, as I see the way in which Module::New::Date use Time::Piece shows me some valid (but tricky) use cases that are now broken. I still have to find if they are really in the wild.

About Module::New::Date, I still think it will be much cleaner with my charsbar/module-new#2 fix.

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Apr 18, 2015

Collaborator

Another issue is the export method has: $class->SUPER::export
Without inheritance, not sure where the call to SUPER will go.

Collaborator

smith153 commented Apr 18, 2015

Another issue is the export method has: $class->SUPER::export
Without inheritance, not sure where the call to SUPER will go.

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Apr 28, 2015

Collaborator

It looks like in version 1.09 the overridden export method was added with comment "Time::Piece should now be safely subclassable"

Collaborator

smith153 commented Apr 28, 2015

It looks like in version 1.09 the overridden export method was added with comment "Time::Piece should now be safely subclassable"

@dolmen

This comment has been minimized.

Show comment
Hide comment
@dolmen

dolmen Apr 28, 2015

Contributor

I recomment to revert b0a7e5a for now, because I broke ->import with that change.
I will provide an alternate implementation later.

Contributor

dolmen commented Apr 28, 2015

I recomment to revert b0a7e5a for now, because I broke ->import with that change.
I will provide an alternate implementation later.

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