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

No warning if optional megasplat not present. #1128

Closed

Conversation

bigpresh
Copy link
Member

@bigpresh bigpresh commented Feb 7, 2016

This is the same bug reported in D1 as PerlDancer/Dancer#1144 - the bug was
also present in D2.

The D1 fix was in commit PerlDancer/Dancer@a5e54dc

This is the same bug reported in D1 as PerlDancer/Dancer#1144 - the bug was
also present in D2.

The D1 fix was in commit PerlDancer/Dancer@a5e54dc
@veryrusty
Copy link
Member

Looks good. Any chance for adding a test? (t/classes/Dancer2-Core-Route/match.t is probably the best place)

@bigpresh
Copy link
Member Author

bigpresh commented Feb 8, 2016

Right, there's tests for the behaviour of optional megasplats with & without a value - they pass before and after this change.

I was wondering if we should have all test scripts check for the absence of warnings too automatically, but it wouldn't have caught this, as we weren't testing an optional megasplat without a value, it seems.

@veryrusty
Copy link
Member

I'd normally use Test::Warnings to test for such warnings (or the lack there of). We don't currently have it as a test dependency. Anyone got any thoughts on using Test::Warnings ?

@xsawyerx
Copy link
Member

xsawyerx commented Feb 8, 2016

@veryrusty Don't we use Capture::Tiny? Let me take a look at this today.

@xsawyerx
Copy link
Member

xsawyerx commented Feb 8, 2016

Okay, we do use Capture::Tiny, which means we can test warnings that way.

@xsawyerx
Copy link
Member

xsawyerx commented Feb 8, 2016

Oh, and 👍 for the PR. @bigpresh++

@veryrusty
Copy link
Member

👍 I'll merge with the Capture::Tiny tests for warnings.

@veryrusty
Copy link
Member

Merged in 74afc7c. @bigpresh++

@veryrusty veryrusty closed this Feb 8, 2016
@veryrusty veryrusty deleted the bigpresh/d1_issue_1144/warning_on_optional_megasplat branch February 8, 2016 10:14
@bigpresh
Copy link
Member Author

bigpresh commented Feb 8, 2016

Cheers!

@Relequestual
Copy link
Contributor

\o/

xsawyerx added a commit that referenced this pull request Apr 19, 2016
    [ BUG FIXES ]
    * GH #1102: Handle multiple '..' in file path utilities.
      (Oleg A. Mamontov, Peter Mottram)
    * GH #1114: Fix missing prereqs as reported by CPANTS.
      (Mohammad S Anwar)
    * GH #1128: Shh warning if optional megasplat is not present.
      (David Precious)
    * GH #1139: Fix incorrect Content-Length header added by AutoPage
      handler (Michael Kröll, Russell Jenkins)
    * GH #1144: Change tt tags to span in skel (Jason Lewis)
    * GH #1046: "no_server_tokens" configuration option doesn't work.
      (Sawyer X)
    # GH #1155, #1157: Fix megasplat value splitting when there are empty
      trailing path segments. (Tatsuhiko Miyagawa, Russell Jenkins)
      NOTE: Paths matching a megasplat that end with a '/' will now include
      an empty string as the last value. For the route pattern '/foo/**',
      the path '/foo/bar', the megasplat gives ['bar'], whereas '/foo/bar/'
      now gives ['bar','']. Joining the array of megasplat values will now
      always be the string matched against for the megasplit.

    [ DOCUMENTATION ]
    * GH #1119: Improve the deployment documentation. (Andrew Beverley)
    * GH #1123: Document import of utf8 pragma. (Victor Adam)
    * GH #1132: Fix spelling mistakes in POD (Gregor Herrmann)
    * GH #1134: Fix spelling errors detected by codespell (James McCoy)
    * GH #1153: Fix POD rendering error. (Sawyer X)

    [ ENHANCEMENTS ]
    * GH #1129: engine.logger.* hooks are called around logging a message.
      (Russell @veryrusty Jenkins)
    * GH #1146: Cleaner display of error context (Vernon Lyon)
    * GH #1085: Add consistent keywords for accessing headers;
      'request_header' for request, 'response_header', 'response_headers'
      and 'push_response_header' for response. (Russell @veryrusty Jenkins)
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

Successfully merging this pull request may close these issues.

None yet

4 participants