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]Clarify priority adjust settings docs #1727

Merged
merged 3 commits into from Jan 27, 2016

Conversation

@redapple
Copy link
Contributor

@redapple redapple commented Jan 26, 2016

Fixes #1593

Fixes #1593
@codecov-io
Copy link

@codecov-io codecov-io commented Jan 26, 2016

Current coverage is 83.29%

Merging #1727 into master will not affect coverage as of 35b87d6

Powered by Codecov. Updated on successful CI builds.

@kmike
Copy link
Member

@kmike kmike commented Jan 26, 2016

what about DEPTH_PRIORITY?

@redapple
Copy link
Contributor Author

@redapple redapple commented Jan 26, 2016

@kmike , do you mean also changing the description for http://doc.scrapy.org/en/latest/topics/settings.html#depth-priority ?

@kmike
Copy link
Member

@kmike kmike commented Jan 26, 2016

yes

@kmike
Copy link
Member

@kmike kmike commented Jan 26, 2016

I wonder if we should rename it to DEPTH_PRIORITY_ADJUST to be consistent with other settings. Though adjustment algorithm is different from other settings (which is also not explained in docs): request.priority -= request.meta['depth'] * DEPTH_PRIORITY_ADJUST .

@redapple
Copy link
Contributor Author

@redapple redapple commented Jan 27, 2016

@kmike , I'd rather we do not change the setting name to DEPTH_PRIORITY_ADJUST as it behave differently to the 2 others.
(we could rename to DEPTH_PRIORITY_NEGATIVE_ADJUST to be explicit but I wonder if it would not confuse more than anything)

@kmike
Copy link
Member

@kmike kmike commented Jan 27, 2016

@redapple a good point.
We can make DEPTH_PRIORITY_ADJUST option an opposite of DEPTH_PRIORITY.

@redapple
Copy link
Contributor Author

@redapple redapple commented Jan 27, 2016

@kmike , is renaming ok in a seperate PR?

@kmike
Copy link
Member

@kmike kmike commented Jan 27, 2016

@redapple sure, improved docs is already a big step forward


An integer that is used to adjust the request priority based on its depth:

- **a positive value will decrease the priority**

This comment has been minimized.

@kmike

kmike Jan 27, 2016
Member

Can we be even more explicit? E.g. "it means requests with larger depth will be processed later".

This comment has been minimized.

@kmike

kmike Jan 27, 2016
Member

I always had troubles reading scrapy docs when priorities are explained :) Sometimes a larger value means 'more priority', sometimes it means 'later in a queue', this is confusing.

@redapple
Copy link
Contributor Author

@redapple redapple commented Jan 27, 2016

ok @kmike , I also added a link to the FAQ entry about BFO/DFO.

@nramirezuy nramirezuy changed the title Clarify priority adjust settings docs [MRG+1]Clarify priority adjust settings docs Jan 27, 2016
@redapple redapple added this to the v1.1 milestone Jan 27, 2016
@redapple redapple added the docs label Jan 27, 2016
redapple added a commit that referenced this pull request Jan 27, 2016
[MRG+1] Clarify priority adjust settings docs
@redapple redapple merged commit 27fb200 into scrapy:master Jan 27, 2016
2 checks passed
2 checks passed
codecov/patch coverage not affected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike kmike mentioned this pull request Jan 27, 2016
@kmike
Copy link
Member

@kmike kmike commented Jan 27, 2016

👍

@redapple redapple deleted the redapple:priority-adjust branch Feb 1, 2016
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

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