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

Add an api option to toggle the debugger #12

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
2 participants
@ghost

ghost commented Feb 29, 2016

No description provided.

@stevan

This comment has been minimized.

Show comment
Hide comment
@stevan

stevan Mar 3, 2016

Owner

Would you mind adding a simple unit test for this?

Owner

stevan commented Mar 3, 2016

Would you mind adding a simple unit test for this?

stevan added a commit that referenced this pull request Mar 8, 2016

Merge pull request #12 from SkySymbol/api_on_off
Add an api option to toggle the debugger

@stevan stevan merged commit e57e119 into stevan:master Mar 8, 2016

@stevan

This comment has been minimized.

Show comment
Hide comment
@stevan

stevan Mar 8, 2016

Owner

Looks good, thanks!

Owner

stevan commented Mar 8, 2016

Looks good, thanks!

@ghost ghost deleted the api_on_off branch Mar 8, 2016

@SysPete

This comment has been minimized.

Show comment
Hide comment
@SysPete

SysPete Aug 9, 2016

Contributor

This commit breaks Plack::App::Debugger:

Error:  Global symbol "$req" requires explicit package name at /home/apm/git/Plack-Debugger/lib/Plack/App/Debugger.pm line 130.

And the new test itself fails to compile after fixing up App::Debugger.

I've cooked up a quick branch https://github.com/SysPete/p5-plack-debugger/tree/fixup_pr_12 but since this PR doesn't appear to actually do anything apart from adding code (in the wrong place) I think a revert is a better option which is why I have not created a PR.

Contributor

SysPete commented Aug 9, 2016

This commit breaks Plack::App::Debugger:

Error:  Global symbol "$req" requires explicit package name at /home/apm/git/Plack-Debugger/lib/Plack/App/Debugger.pm line 130.

And the new test itself fails to compile after fixing up App::Debugger.

I've cooked up a quick branch https://github.com/SysPete/p5-plack-debugger/tree/fixup_pr_12 but since this PR doesn't appear to actually do anything apart from adding code (in the wrong place) I think a revert is a better option which is why I have not created a PR.

@stevan

This comment has been minimized.

Show comment
Hide comment
@stevan

stevan Aug 10, 2016

Owner

@SysPete actually your patch does fix it, please make it a proper pull request.

It removes the return $req, which is wrong anyway, it should be return ($req, undef), which is what happens anyway at the bottom of the subroutine if you remove those lines. Also, moving it down several lines means that $req is in scope and won't have that error you were seeing.

Sorry, this was my fault, I didn't inspect the patch/pull-request well enough. Thankfully you caught this before it went out to CPAN.

Owner

stevan commented Aug 10, 2016

@SysPete actually your patch does fix it, please make it a proper pull request.

It removes the return $req, which is wrong anyway, it should be return ($req, undef), which is what happens anyway at the bottom of the subroutine if you remove those lines. Also, moving it down several lines means that $req is in scope and won't have that error you were seeing.

Sorry, this was my fault, I didn't inspect the patch/pull-request well enough. Thankfully you caught this before it went out to CPAN.

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