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

[MRG+1] Allowed passing objects of Mapping class or its subclass to the CaselessDict initializer #2646

Merged
merged 2 commits into from Mar 15, 2017

Conversation

@woxcab
Copy link
Contributor

@woxcab woxcab commented Mar 13, 2017

CaselessDict can process sequence of general Mapping/MutableMapping class or subclass of Mapping.

Documentations both for Python 2 and Python 3 declare that all instances of Mapping class have items() method, so it's safely to generalize check from isinstance(seq, dict).

Real usage example: requests package returns responses which headers are instances of requests.structures.CaseInsensitiveDict that is subclass of MutableMapping and this patch allows to pass these headers directly to scrapy.Response initializer.

…essDict initializer
@kmike
Copy link
Member

@kmike kmike commented Mar 14, 2017

The feature make sense.

I wonder if we should go further and just check for
hasattr(seq, 'items') and callable(seq.items). Thoughts?

@woxcab
Copy link
Contributor Author

@woxcab woxcab commented Mar 14, 2017

I wonder if we should go further and just check for
hasattr(seq, 'items') and callable(seq.items). Thoughts?

Generic items() method can have another semantic and can return anything. But Mapping.items() behaviour is limited and specified by documentation and it's currently the most generic form of required behaviour, so I think the current version of pull request is more appropriate.

@kmike
Copy link
Member

@kmike kmike commented Mar 14, 2017

ok, makes sense!

seq = MyMutableMapping(red=1, black=3)
d = CaselessDict(seq)
self.assertEqual(d['red'], 1)
self.assertEqual(d['black'], 3)

This comment has been minimized.

@kmike

kmike Mar 14, 2017
Member

Could you please split it into several separate methods? Original test_init implementation was not a good example, it should have been two methods in a first place.

This comment has been minimized.

@woxcab

woxcab Mar 15, 2017
Author Contributor

@kmike, into which semantic methods to split this method? I.e. what is the criterion for the splitting?

This comment has been minimized.

@kmike

kmike Mar 15, 2017
Member

by input to the CaselessDict constructor: dict, sequence, Mapping, MutableMapping

This comment has been minimized.

@woxcab

woxcab Mar 15, 2017
Author Contributor

Completed.

This comment has been minimized.

@kmike

kmike Mar 15, 2017
Member

Thanks @woxcab, looks good!

@kmike kmike changed the title Allowed passing objects of Mapping class or its subclass to the CaselessDict initializer [MRG+1] Allowed passing objects of Mapping class or its subclass to the CaselessDict initializer Mar 14, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Mar 15, 2017

Codecov Report

Merging #2646 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2646      +/-   ##
==========================================
+ Coverage   84.25%   84.26%   +0.01%     
==========================================
  Files         162      162              
  Lines        9106     9106              
  Branches     1350     1350              
==========================================
+ Hits         7672     7673       +1     
  Misses       1174     1174              
+ Partials      260      259       -1
Impacted Files Coverage Δ
scrapy/utils/datatypes.py 60.21% <100%> (ø)
scrapy/utils/trackref.py 86.48% <0%> (+2.7%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfb5640...a84652e. Read the comment docs.

@kmike kmike added this to the v1.4 milestone Mar 15, 2017
@redapple redapple merged commit 1c0da17 into scrapy:master Mar 15, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.