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

Add is_sequence_with matcher #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErwinJunge
Copy link
Contributor

This adds a matcher called is_sequence_with to allow partial list matches (i.e. allow extra items before and/or after).

@ErwinJunge
Copy link
Contributor Author

FYI: the (remaining) travis errors are not related to the code changes in this PR, they have something to do with travis setting up py32 and py33

@ErwinJunge
Copy link
Contributor Author

Now that #13 is merged, we should probably continue with this one :)

FYI, I'm going on vacation for 3 weeks, so I can pick this up in late august at the earliest. Feel free to give your feedback already, I just wanted to let you know I won't have time to look at it before coming back from vacation.

@mwilliamson
Copy link
Owner

Apologies for the slow response on this one, I'd completely understand if you've moved on from this! Still, I thought it was worth sharing thoughts in case anyone (myself included!) wants to pick this up.

  1. For consistency with other matchers, I'd suggest renaming this to includes_sequence()
  2. If I've understood correctly, this produces the same description for includes_sequence() as for is_sequence(), whereas different matchers should have different descriptions
  3. Because the expected sequence could be anywhere in the actual sequence, the best error message to display for a mismatch is unclear. It'd probably be useful to have either, or both of, the full actual sequence, and a mismatch description for the closest match
  4. This feels sufficiently distinct from is_sequence that I'd suggest just splitting out the implementation entirely, and extracting the common parts (if any)

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