bpo-35922: Fix RobotFileParser when robots.txt is invalid by remilapeyre · Pull Request #11791 · python/cpython

remilapeyre · 2019-02-08T16:05:31Z

I don't think this change requires a news entry.

https://bugs.python.org/issue35922

taleinat

In addition to testing with empty robots.txt content, it would be good to also test with:

one not referencing the given user-agent and without a default
one with a default or with the given user-agent, but which doesn't define Crawl_delay and Request_rate

bedevere-bot · 2019-06-08T19:33:46Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-Authored-By: Tal Einat <taleinat+github@gmail.com>

remilapeyre · 2019-06-09T16:49:27Z

Thanks for the review @taleinat! I made all the changes you requested, let me know if you want me to make other updates.

I have made the requested changes; please review again

bedevere-bot · 2019-06-09T16:49:31Z

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

taleinat

These changes are very well done, and they were done very quickly; nice!

Taking a deeper look at the structure of the existing tests, we would do better to build on the existing BaseRequestRateTest. Specifically, I think its test_request_rate() method should be changed to always check the crawl delay and request rate, so that setting them to None will check that RobotFileParser actually returns None.

Doing so should allow removing NoDefaultUserAgentTest, since that case will already be covered by the existing CrawlDelayAndRequestRateTest. EmptyFileTest should stay but not require any special logic, just define robots_txt to an empty stings. Similarly, NoRequestRateAndCrawlDelayTest could be written much more simply.

Let me know if you'd like to do this yourself or if you'd prefer me to make these changes myself.

bedevere-bot · 2019-06-09T19:24:08Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

remilapeyre · 2019-06-09T19:41:55Z

Yeah, it will be better that way, I will push a new commit tomorrow.

remilapeyre · 2019-06-11T09:35:30Z

Thanks @taleinat, I have made the requested changes; please review again

bedevere-bot · 2019-06-11T09:35:34Z

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

miss-islington · 2019-06-12T10:13:20Z

@remilapeyre: Status check is done, and it's a failure ❌ .

taleinat · 2019-06-12T10:14:28Z

Not sure what the status check is, @taleinat should I backport manually?

No, please ignore those messages. They seem to be caused by a bug in Miss Islington.

miss-islington · 2019-06-12T10:17:49Z

@remilapeyre: Status check is done, and it's a failure ❌ .

taleinat · 2019-06-12T10:21:22Z

Not sure what the status check is, @taleinat should I backport manually?

No, please ignore those messages. They seem to be caused by a bug in Miss Islington.

Or maybe Miss Islington was just made aware of the failure earlier...

ZackerySpytz · 2019-06-12T10:23:22Z

The commit message should use "robots.txt", not "robot.txt".

berkerpeksag

Good catch! I just left very minor comments. Otherwise it looks good to me.

berkerpeksag · 2019-06-12T15:00:40Z

-        return self.default_entry.delay
+        if self.default_entry:
+            return self.default_entry.delay
+        else:


The else branch isn't needed:

if self.default_entry: return self.default_entry.delay return None

berkerpeksag · 2019-06-12T15:00:50Z

-        return self.default_entry.req_rate
+        if self.default_entry:
+            return self.default_entry.req_rate
+        else:


Same as above.

berkerpeksag · 2019-06-12T15:01:33Z

@@ -0,0 +1,4 @@
+Fix :meth:`RobotFileParser.crawl_delay()` and


Nit: () can be removed. Sphinx will it add it automatically.

berkerpeksag · 2019-06-12T15:01:47Z

@@ -0,0 +1,4 @@
+Fix :meth:`RobotFileParser.crawl_delay()` and
+:meth:`RobotFileParser.request_rate()` to return ``None`` rather than


Nit: () can be removed. Sphinx will it add it automatically.

berkerpeksag · 2019-06-12T15:02:23Z



+class EmptyFileTest(BaseRequestRateTest, unittest.TestCase):
+    robots_txt = ""


Nit: Please use single quotes.

bedevere-bot · 2019-06-12T15:04:28Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

taleinat · 2019-06-12T18:35:25Z

I have made the requested changes; please review again.

taleinat · 2019-06-14T13:38:00Z

@berkerpeksag, just awaiting your final okay, and I'll go about merging and backporting this.

berkerpeksag

Good catch! Thank you for fixing this.

miss-islington · 2019-06-16T06:49:00Z

Thanks @remilapeyre for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-06-16T06:49:25Z

GH-14121 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2019-06-16T06:49:32Z

GH-14122 is a backport of this pull request to the 3.7 branch.

…delay or request rate (pythonGH-11791) Co-Authored-By: Tal Einat <taleinat+github@gmail.com> (cherry picked from commit 8047e0e) Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>

…delay or request rate (GH-11791) Co-Authored-By: Tal Einat <taleinat+github@gmail.com> (cherry picked from commit 8047e0e) Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>

…delay or request rate (pythonGH-11791) Co-Authored-By: Tal Einat <taleinat+github@gmail.com>

Fix RobotFileParser when robot.txt is invalid

e7b777f

the-knights-who-say-ni added the CLA signed label Feb 8, 2019

bedevere-bot added the awaiting review label Feb 8, 2019

auvipy approved these changes Jun 2, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jun 2, 2019

taleinat requested changes Jun 8, 2019

View reviewed changes

Comment thread Lib/test/test_robotparser.py Outdated

Comment thread Lib/urllib/robotparser.py

bedevere-bot added awaiting changes and removed awaiting core review labels Jun 8, 2019

Rename test case

decb931

Co-Authored-By: Tal Einat <taleinat+github@gmail.com>

remilapeyre requested a review from berkerpeksag as a code owner June 9, 2019 16:30

Rémi Lapeyre added 2 commits June 9, 2019 18:35

Return None explicitly

b219f9d

Add tests for bad user-agent and missing attributes

f417727

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 9, 2019

taleinat requested changes Jun 9, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jun 9, 2019

Always test craw_delay and request_rate

00b918a

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 11, 2019

taleinat reviewed Jun 11, 2019

View reviewed changes

Comment thread Lib/test/test_robotparser.py Outdated

taleinat requested changes Jun 11, 2019

View reviewed changes

Comment thread Lib/test/test_robotparser.py Outdated

fix NEWS entry formatting

d2e9b69

taleinat changed the title ~~bpo-35922: Fix RobotFileParser when robot.txt is invalid~~ bpo-35922: Fix RobotFileParser when robots.txt is invalid Jun 12, 2019

taleinat removed the 🤖 automerge label Jun 12, 2019

berkerpeksag requested changes Jun 12, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jun 12, 2019

address Berker Peksag's review comments

67a6ee2

berkerpeksag approved these changes Jun 15, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jun 15, 2019

taleinat merged commit 8047e0e into python:master Jun 16, 2019

bedevere-bot removed the awaiting merge label Jun 16, 2019

bedevere-bot removed the needs backport to 3.8 label Jun 16, 2019

bedevere-bot removed the needs backport to 3.7 label Jun 16, 2019

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-35922: Fix RobotFileParser when robots.txt has no relevant crawl …

a181539

…delay or request rate (pythonGH-11791) Co-Authored-By: Tal Einat <taleinat+github@gmail.com>

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-35922: Fix RobotFileParser when robots.txt has no relevant crawl …

bf82f9b

…delay or request rate (pythonGH-11791) Co-Authored-By: Tal Einat <taleinat+github@gmail.com>

		@@ -0,0 +1,4 @@
		Fix :meth:`RobotFileParser.crawl_delay()` and

		@@ -0,0 +1,4 @@
		Fix :meth:`RobotFileParser.crawl_delay()` and
		:meth:`RobotFileParser.request_rate()` to return ``None`` rather than



		class EmptyFileTest(BaseRequestRateTest, unittest.TestCase):
		robots_txt = ""

Uh oh!

Uh oh!

Conversation

remilapeyre commented Feb 8, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 8, 2019

Uh oh!

remilapeyre commented Jun 9, 2019

Uh oh!

bedevere-bot commented Jun 9, 2019

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 9, 2019

Uh oh!

remilapeyre commented Jun 9, 2019

Uh oh!

remilapeyre commented Jun 11, 2019

Uh oh!

bedevere-bot commented Jun 11, 2019

Uh oh!

Uh oh!

Uh oh!

miss-islington commented Jun 12, 2019

Uh oh!

taleinat commented Jun 12, 2019

Uh oh!

miss-islington commented Jun 12, 2019

Uh oh!

taleinat commented Jun 12, 2019

Uh oh!

ZackerySpytz commented Jun 12, 2019

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 12, 2019

Uh oh!

taleinat commented Jun 12, 2019

Uh oh!

taleinat commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jun 16, 2019

Uh oh!

bedevere-bot commented Jun 16, 2019

Uh oh!

bedevere-bot commented Jun 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

remilapeyre commented Feb 8, 2019 •

edited by bedevere-bot

Loading

taleinat commented Jun 14, 2019 •

edited

Loading