Add mapping_includes matcher #13
Conversation
|
FYI: the travis errors are not related to the code changes in this PR, they have something to do with travis setting up py32 and py33. |
|
Thanks for the pull request. Rebasing onto master should fix the Python 3.2 and 3.3 errors (by dropping support). I'm not sure the naming of the function is very clear. We already have an |
precisely/mapping_matchers.py
Outdated
| @@ -3,16 +3,21 @@ | |||
| from .results import matched, unmatched, indented_list | |||
|
|
|||
|
|
|||
| def is_mapping(matchers): | |||
| def is_mapping(matchers, allow_extra=False): | |||
There was a problem hiding this comment.
If we're creating a separate function to allow extra keys, I don't think we should allow the argument in is_mapping().
There was a problem hiding this comment.
I went with the kwarg for 2 reasons:
- maximize code reuse
- it makes
is_mapping_withinto syntactic sugar aroundis_mapping
Both of those seemed to me to be good reasons for this structure, but if you prefer that I copypaste and modify is_mapping I'll gladly do so.
tests/is_mapping_with_tests.py
Outdated
| @istest | ||
| def description_describes_keys_and_value_matchers(): | ||
| matcher = is_mapping_with({"a": equal_to(1), "b": equal_to(2)}) | ||
| assert_equal("mapping with items:\n * 'a': 1\n * 'b': 2", matcher.describe()) |
There was a problem hiding this comment.
This description is the same as would be produced with is_mapping(), but there should be some way of distinguishing the two.
There was a problem hiding this comment.
Good point, modified.
ErwinJunge
force-pushed
the
feature/is_mapping_with
branch
from
219d48e to
ded9c01
Compare
Rebased. The naming ties into #14 where I introduce |
|
I'm not sure |
README.rst
Outdated
| @@ -117,6 +117,18 @@ For instance, ``has_attrs(name="bob")`` is equivalent to ``has_attrs(name=equal_ | |||
| "b": equal_to(4), | |||
| })) | |||
|
|
|||
| * ``is_mapping_with(**matchers)``: matches a mapping, such as a ``dict``, if it has the same keys with matching values. | |||
There was a problem hiding this comment.
I think the arguments here are matchers rather than **matchers (I made a mistake, now fixed, when documenting is_mapping())
There was a problem hiding this comment.
Good point, modified.
ErwinJunge
force-pushed
the
feature/is_mapping_with
branch
from
ded9c01 to
a69aa4a
Compare
precisely/mapping_matchers.py
Outdated
| return "mapping including items:{0}".format(indented_list(sorted( | ||
| "{0!r}: {1}".format(key, matcher.describe()) | ||
| for key, matcher in self._matchers.items() | ||
| ))) |
There was a problem hiding this comment.
The split in logic here feels awkward to me: half the specialisation for mapping_includes() is in this subclass, while the other half is in the superclass. I think it'd be simpler to switch on self._allow_extra in IsMappingMatcher.describe(). Other than that, I think this looks good.
There was a problem hiding this comment.
I went with a slightly more subclassing-based approach and got rid of _allow_extra entirely. What do you think of the latest push?
There was a problem hiding this comment.
I tend to avoid implementation inheritance (or more specifically the template pattern). I'd suggest either switching on self._allow_extra, or creating two separate classes that both delegate to a common matching function.
There was a problem hiding this comment.
I've put self._allow_extra back in.
ErwinJunge
force-pushed
the
feature/is_mapping_with
branch
from
a69aa4a to
c0f4883
Compare
ErwinJunge
force-pushed
the
feature/is_mapping_with
branch
from
c0f4883 to
d4476ce
Compare
|
Thanks! I've merged this, and made a couple of minor tweaks to the implementation. |
This adds a matcher called is_mapping_with to allow partial dict matches (i.e. allow extra keys).