Skip to content

Conversation

@ruz
Copy link
Contributor

@ruz ruz commented Nov 6, 2012

This pull request is a request for review and comments.

It fixes several issues reported earlier, but a few test start to fail. Could you please review failing tests and decide how to change tests and/or code?

ruz added 2 commits November 6, 2012 12:20
Not only " should be escaped, but \ as well.
Before this change each matched mailbox in the string
was re-matched with separate regexps to extract parts.
This could lead to very suprising results. See ticket #52102
for details [1].

Also, quoted phrases was not de-quoted properly.

[1] https://rt.cpan.org/Ticket/Display.html?id=52102
@rjbs
Copy link
Collaborator

rjbs commented Nov 11, 2012

I believe that the failing test cases are test bugs, not code bugs in your changes.

Also, I loathe these tests. For example, this is terrible:

[
'"Name"',
...
]

The first argument there is fed to build a new address against which to compare the parsed one, so the double-quotes are redundant. It would be much better if the test values were tests for the results of the phrase, address, comment, as_string, and name methods. This would break tests with dumb extra quotes, but would make the meaning of the tests much easier. I'd actually like the tests to be more in the form:

$tests = {
  q{"Some Input" <string@example.com>} => [
    {
      as_string => ...defaults to "should be $input"...,
      phrase => "Some Input",
      address => 'string@example.com',
    }
  ],
  ...,
}

...maybe I'll do that after you're done, unless you build something from that Dominic Sayers corpus. ;)

@rjbs
Copy link
Collaborator

rjbs commented Nov 11, 2012

I pushed a few small changes, primarily to make it easier for me to skim test output.

@rjbs
Copy link
Collaborator

rjbs commented Aug 2, 2013

You wrote:

This pull request is a request for review and comments.

I reviewed, I think. What's left for this to be merged? Test fixes?

@ruz
Copy link
Contributor Author

ruz commented Aug 26, 2013

test fixes.

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.

2 participants