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

Dupefilter request_fingerprint test and docs #597

Merged

Conversation

@chekunkov
Copy link
Contributor

@chekunkov chekunkov commented Feb 15, 2014

This PR is related to #533, title is pretty self-explanatory, changes are minor. Please check test and grammar.

One more thing I was thinking about - maybe it would be better to rename request_fingerprint method to get_request_fingerprint? Can name clash between RFPDupeFilter method and scrapy.utils.request.request_fingerprint function cause confusion to people? What do you think?

@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Feb 16, 2014

LGTM.

One thing about changing the name of request_fingerprint: that function is from scrapy.utils.request module whose functions have a convention of starting with request_ prefix.

@chekunkov
Copy link
Contributor Author

@chekunkov chekunkov commented Feb 17, 2014

No no no, I said

to rename request_fingerprint method

meaning RFPDupeFilter.request_fingerprint, not request_fingerprint function from scrapy.utils.request. Sorry about that ambiguity, and btw this is exactly what I meant talking about name clash and confusion.

@kmike
Copy link
Member

@kmike kmike commented Feb 17, 2014

I think changing request_fingerprint to get_request_fingerprint doesn't worth it: it is backwards incompatible, and I don't see how they can get people confused in practice - you can't occasionally override them by importing, for example. "Naming clash" may happen only in discussions, when somebody omits class name - this doesn't look like a problem we should fix.

the ``scrapy.utils.request.request_fingerprint`` function.
the ``scrapy.utils.request.request_fingerprint`` function. In order to change
the way duplicates are checked you could subclass ``RFPDupeFilter`` and
override its ``request_fingerprint`` method.

This comment has been minimized.

@kmike

kmike Feb 17, 2014
Member

To understand how to change the way duplicates are checked users still have to look at the source code because request_fingerprint interface is not documented. Maybe add something like "This method should accept scrapy Request instance and return its fingerprint (a string)"?

@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Apr 25, 2014

I'm happy to merge this, could you rebase it @chekunkov?.

It'd be better if we had the dupe filter interface documented too, but it can be a separate PR.

@chekunkov
Copy link
Contributor Author

@chekunkov chekunkov commented Apr 26, 2014

@pablohoffman done. I've rebased branch on top of the master + added short RFPDupeFilter.request_fingerprint interface description (just like @kmike suggested)

dangra added a commit that referenced this pull request Apr 28, 2014
…_test_and_docs

Dupefilter request_fingerprint test and docs
@dangra dangra merged commit ad14cf0 into scrapy:master Apr 28, 2014
1 check passed
1 check passed
continuous-integration/travis-ci 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