mjg/perl-Net-SFTP-Foreign-Exceptional
1
0
Fork 0
Code Issues 2 Pull requests Projects Releases Packages Wiki Activity Actions

don't use inheritance for that! #1

Closed
salva wants to merge 5 commits from master into master
pull from: master
merge into: mjg:master
Conversation 3 Commits 5 Files changed 2 +42 -128
salva commented 2011-05-17 13:19:47 +00:00 (Migrated from github.com)
Copy link

Using inheritance for that is a bad idea and will break several of the module methods.

That happens because your module changes the API (contract) of Net::SFTP::Foreign and this module its a client of itself that calls its own methods and expects them to behave in the former way and not to die on error but just to set the error condition.

I have changed your module to use a wrapper instead.

Using inheritance for that is a bad idea and will break several of the module methods. That happens because your module changes the API (contract) of Net::SFTP::Foreign and this module its a client of itself that calls its own methods and expects them to behave in the former way and not to die on error but just to set the error condition. I have changed your module to use a wrapper instead.
mjgardner commented 2011-05-18 15:27:31 +00:00 (Migrated from github.com)
Copy link

I couldn't merge your changes automatically, and goto and AUTOLOAD make me itch so I used delegation. Does this look acceptable?

mjgardner/net-sftp-foreign-exceptional@c1098beb51

I couldn't merge your changes automatically, and goto and AUTOLOAD make me itch so I used delegation. Does this look acceptable? mjgardner/net-sftp-foreign-exceptional@c1098beb513adaa2832f
salva commented 2011-05-18 16:49:48 +00:00 (Migrated from github.com)
Copy link

From: mjgardner
reply@reply.github.com
To: sfandino@yahoo.com
Sent: Wed, May 18, 2011 5:27:32 PM
Subject: Re: [net-sftp-foreign-exceptional] don't use inheritance for that! (#1)

I couldn't merge your changes automatically, and goto and AUTOLOAD make me itch
so I used delegation. Does this look acceptable?

Delegation is what the AUTOLOAD approach does, it just builds efficient proxy
methods on demand and installs them on the namespace of the wrapping module.

The goto is not really a goto as in the classic "Go To Statement Considered
Harmful" article
(http://www.u.arizona.edu/~rubinson/copyright_violations/Go_To_Considered_Harmful.html).
It is a subroutine call with tail-call optimization
(http://en.wikipedia.org/wiki/Tail_call). You can remove the goto keyword and it
will continue to work exactly the same.

Besides that, yours solution depends on more than 30 external packages, eats
lots of memory and takes eons to load, just for the sake of using "modern Perl"
practices instead of the old good autoload... that makes me itch!

Anyway, it is your module and it is up to you to decide how to program it, I
know that being accustomed to Moose is difficult to refrain from its usage,
specially when your applications are already using it anyway.

There is a bug in your code, Class::Inspector::methods do not returns a list but
an array reference. You also need to remove "new" and "DESTROY" from the list.

  • Salva

Reply to this email directly or view it on GitHub:
https://github.com/mjgardner/net-sftp-foreign-exceptional/pull/1#issuecomment-1197661

--- From: mjgardner reply@reply.github.com To: sfandino@yahoo.com Sent: Wed, May 18, 2011 5:27:32 PM Subject: Re: [net-sftp-foreign-exceptional] don't use inheritance for that! (#1) > I couldn't merge your changes automatically, and goto and AUTOLOAD make me itch > so I used delegation. Does this look acceptable? Delegation is what the AUTOLOAD approach does, it just builds efficient proxy methods on demand and installs them on the namespace of the wrapping module. The goto is not really a goto as in the classic "Go To Statement Considered Harmful" article (http://www.u.arizona.edu/~rubinson/copyright_violations/Go_To_Considered_Harmful.html). It is a subroutine call with tail-call optimization (http://en.wikipedia.org/wiki/Tail_call). You can remove the goto keyword and it will continue to work exactly the same. Besides that, yours solution depends on more than 30 external packages, eats lots of memory and takes eons to load, just for the sake of using "modern Perl" practices instead of the old good autoload... _that_ makes me itch! Anyway, it is your module and it is up to you to decide how to program it, I know that being accustomed to Moose is difficult to refrain from its usage, specially when your applications are already using it anyway. There is a bug in your code, Class::Inspector::methods do not returns a list but an array reference. You also need to remove "new" and "DESTROY" from the list. - Salva ## Reply to this email directly or view it on GitHub: https://github.com/mjgardner/net-sftp-foreign-exceptional/pull/1#issuecomment-1197661
mjgardner commented 2011-05-18 17:50:47 +00:00 (Migrated from github.com)
Copy link

Yes, the app I use this in already uses Moose. Thanks for the feedback!

Yes, the app I use this in already uses Moose. Thanks for the feedback!

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
housekeeping
No labels
housekeeping
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
mjg/perl-Net-SFTP-Foreign-Exceptional!1
No description provided.