Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Dancer2 doesn't include the local ./lib directory by default #481

Closed
sukria opened this Issue Sep 26, 2013 · 16 comments

Comments

Projects
None yet
7 participants
Owner

sukria commented Sep 26, 2013

With Dancer 1, it's possible to do that:

use Dancer;
use SomeApp;
dance;

Where lib/SomeApp.pm exists.

It doesn't work anymore (used to work) with Dancer2. Apparently, it's a deliberate choice: 7ae4eab

The commit message here is not very explicit about the reason why we break that. I'd like to understand the rationale behind.

Was it causing any hurt somewhere?

I think it was a good thing to have and would like to understand why we got rid of it.

Thanks for the clarification (and sorry if I missed an email/discussion about that).

@ghost ghost assigned xsawyerx Sep 26, 2013

Owner

xsawyerx commented Sep 26, 2013

This is something we've set to revert. It was a mistake. We could push this to be out this weekend.

Contributor

azawawi commented Sep 26, 2013

Is this related also to #478?

Owner

ambs commented Sep 26, 2013

/me bangs his head.

Contributor

sdeseille commented Sep 26, 2013

Hello Alberto

Don't be too hard with you.
Dancer's team make a great work.

Best regards

sdeseille

2013/9/26 Alberto Simões notifications@github.com

/me bangs his head.


Reply to this email directly or view it on GitHubhttps://github.com/PerlDancer/Dancer2/issues/481#issuecomment-25181882
.

Cordialement

Sebastien Deseille

Owner

sukria commented Sep 27, 2013

@xsawyerx okay, then let's put it back on stage when we can :)

Owner

sukria commented Sep 28, 2013

Ok, I'll patch that one and release, we need it to be fixed, I just realized it also breaks with a scaffolded app.

@sukria sukria pushed a commit that referenced this issue Sep 28, 2013

Alexis Sukrieh Fix issue #481 - include local lib by default
When the worker starts, it will always add the local ./lib directory in Perl's
loading path.
05e3118
Contributor

shadowcat-mst commented Sep 29, 2013

given app.pl is now doing the 'use lib' I can't see why Dancer2 should do that. Modifying @inc should be explicit - 05e3118 seems like a step back to me, since it means that if I do e.g.

use lib "/some/path/to/my/app";
use Dancer2;
use SomeApp;

in a script, the script could break depending on the current working directory, which seems utterly horrible.

Contributor

shumphrey commented Sep 29, 2013

I concur with @shadowcat-mst seems to me like app.pl is enough. Whilst I realise this breaks existing apps with old app.pl scripts, it seems like 0560955 is the better solution.

Owner

sukria commented Sep 30, 2013

Okay then, let's put that responsibility on the runner script.

Change to be reverted and documented.

Contributor

shadowcat-mst commented Sep 30, 2013

On Mon, Sep 30, 2013 at 03:44:04AM -0700, Alexis Sukrieh wrote:

Okay then, let's put that responsibility on the runner script.

Change to be reverted and documented.

\o/ Thank you!

Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN
commercial support, training and consultancy packages could help your team.

Contributor

shumphrey commented Sep 30, 2013

@sukria 👍

Contributor

shadowcat-mst commented Oct 5, 2013

On Mon, Sep 30, 2013 at 03:44:04AM -0700, Alexis Sukrieh wrote:

Okay then, let's put that responsibility on the runner script.

Change to be reverted and documented.

Your nasty hack is still in master. Please actually revert it :(

Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN
commercial support, training and consultancy packages could help your team.

@sukria sukria pushed a commit that referenced this issue Oct 11, 2013

Alexis Sukrieh Revert "Fix issue #481 - include local lib by default"
This reverts commit 05e3118.
a005076
Owner

xsawyerx commented Nov 20, 2013

Can we now close this?

Contributor

shumphrey commented Nov 21, 2013

👍

Owner

xsawyerx commented Dec 7, 2013

@sukria closable?

Owner

xsawyerx commented Apr 24, 2014

Time to close this.

@xsawyerx xsawyerx closed this Apr 24, 2014

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