Skip to content

No warning if optional megasplat not present. - #1128

Closed
bigpresh wants to merge 2 commits into
masterfrom
bigpresh/d1_issue_1144/warning_on_optional_megasplat
Closed

bigpresh wants to merge 2 commits into
masterfrom
bigpresh/d1_issue_1144/warning_on_optional_megasplat

Conversation

@bigpresh

@bigpresh bigpresh commented Feb 7, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@bigpresh

bigpresh commented Feb 8, 2016

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

xsawyerx commented Feb 8, 2016

Copy link
Copy Markdown
Member

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

@xsawyerx

xsawyerx commented Feb 8, 2016

Copy link
Copy Markdown
Member

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

@xsawyerx

xsawyerx commented Feb 8, 2016

Copy link
Copy Markdown
Member

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

@veryrusty

Copy link
Copy Markdown
Member

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

@veryrusty

Copy link
Copy Markdown
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

bigpresh commented Feb 8, 2016

Copy link
Copy Markdown
Member Author

Cheers!

@Relequestual

Copy link
Copy Markdown
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.

4 participants