bpo-21475: Support the Sitemap extension in robotparser #6883

mcscope · 2018-05-15T21:55:16Z

This ticket has been open for 3 years just because it was awaiting tests. I took the existing patch and added a test

https://bugs.python.org/issue21475

the-knights-who-say-ni · 2018-05-15T21:55:19Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again to your contribution and we look forward to looking at it!

mcscope · 2018-05-15T22:04:33Z

I signed the CLA

Mariatta

Thanks for the PR. I have several comments (quite nitpicky) but the rest looks good.
In addition, I would suggest adding both yours and Peter's name into Misc/ACKs file.

Mariatta · 2018-05-16T02:03:37Z

Doc/library/urllib.robotparser.rst

+
+      .. versionadded:: 3.8
+
+


Sorry for being so picky, but we only need two extra spaces between .. versionadded and The following example... So please remove the extra lines.

Mariatta · 2018-05-16T02:05:44Z

Misc/NEWS.d/next/Library/2018-05-15-15-03-48.bpo-28612.E9dz39.rst

@@ -0,0 +1,2 @@
+Added support for optional Site Map extension to urllib robotparser. Patch
+by Lady Red


Please end the sentence with a period. In addition, since it was based off another person's patch, it would be good to also mention Based on patch by Peter Wirtz.

bedevere-bot · 2018-05-16T02:07:40Z

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.

Mariatta · 2018-05-16T02:08:55Z

If I don't end up merging this, I'd suggest the core dev merging to remember to add "Co-authored by: Peter Wirtz" in the commit message, since it seems like this was based off Peter's patch.

mcscope · 2018-05-16T04:14:52Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-16T04:14:54Z

Thanks for making the requested changes!

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

mcscope · 2018-05-16T04:15:51Z

Yes please give credit to Peter in the commit message. PS this is my first contribution to cpython! \o/

berkerpeksag · 2018-05-16T04:44:05Z

Lib/test/test_robotparser.py

+    """
+    good = ['/', '/test.html']
+    bad = ['/cyberworld/map/index.html']
+    site_maps = ["http://www.gstatic.com/s2/sitemaps/profiles-sitemap.xml",


Style nit: Please use single quotes.

berkerpeksag · 2018-05-16T04:44:50Z

Lib/test/test_robotparser.py

@@ -292,7 +313,7 @@ def setUp(self):
            # Short poll interval to make the test finish quickly.
            # Time between requests is short enough that we won't wake
            # up spuriously too many times.
-            kwargs={'poll_interval':0.01})
+            kwargs={'poll_interval': 0.01})


Please don't make unrelated cosmetic changes.

oh drat, it's my pep8 autoformatter doing that automatically. will remove

berkerpeksag · 2018-05-16T04:44:56Z

Lib/test/test_robotparser.py

@@ -353,5 +374,6 @@ def test_read_404(self):
        self.assertIsNone(parser.crawl_delay('*'))
        self.assertIsNone(parser.request_rate('*'))

-if __name__=='__main__':
+
+if __name__ == '__main__':


Please don't make unrelated cosmetic changes.

berkerpeksag · 2018-05-16T04:49:26Z

Lib/urllib/robotparser.py

@@ -189,6 +196,11 @@ def request_rate(self, useragent):
                return entry.req_rate
        return self.default_entry.req_rate

+    def site_maps(self):
+        if not self.sitemaps:


Could you also add a test for this branch?

I believe this branch is tested by test_site_maps on all the other tests for robotparser - they each test that it is none except for my single class that tests the positive case

Oops, you're correct, I didn't click to the expand button and didn't notice that the test_site_maps method is part of BaseRobotTest.

berkerpeksag · 2018-05-16T04:55:23Z

Misc/NEWS.d/next/Library/2018-05-15-15-03-48.bpo-28612.E9dz39.rst

@@ -0,0 +1,2 @@
+Added support for optional Site Map extension to urllib robotparser. Patch by


It would be nice to add some links to the new site_maps (:meth:`RobotFileParser.site_maps() <urllib.robotparser.RobotFileParser.site_maps>` -- untested, you'll need to try it locally :)) method or to the urllib.robotparser (:mod:`urllib.robotparser`) module.

bedevere-bot · 2018-05-16T04:55:41Z

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.

…to robotparser_site_maps Conflicts: Misc/NEWS.d/next/Library/2018-05-15-15-03-48.bpo-28612.E9dz39.rst

mcscope · 2018-05-16T05:25:26Z

@berkerpeksag I added the link to the news as you suggested but I can't find any documentation in the dev guide that explains how to do whatever build step I need to do to build the news to evaluate that link. Mind pointing me to the right place?

mcscope · 2018-05-16T05:25:35Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-16T05:25:38Z

Thanks for making the requested changes!

@Mariatta, @berkerpeksag: please review the changes made to this pull request.

mcscope · 2018-05-16T05:35:55Z

Oh, I think I figured out how to make the news, I have to just make the documentation right?

mcscope · 2018-05-16T05:44:13Z

Successfully Tested! The news link works

berkerpeksag · 2018-05-16T06:37:18Z

@mcscope I assume you've found https://devguide.python.org/committing/#what-s-new-and-news-entries but I will share it anyway in case someone else wonders how to build the docs :)

berkerpeksag

LGTM, thank you for helping to finish bpo-21475. This was in my TODO list along with other urllib.robotparser issues but I couldn't find the time to work on them.

ned-deily · 2018-05-16T14:55:53Z

Congratulations on your first cpython PR, @mcscope!

mcscope · 2018-05-17T05:43:22Z

@berkerpeksag Any other todo-list items you have I could take care of? I'm looking at bpo but having a hard time finding ones that require code, instead of requiring developer consensus.

mcscope added 2 commits May 15, 2018 14:33

Replicate original patch by pwirtz, bumping version number

8a71766

Add unit test to test that robot parser correctly parses sitemaps

834367e

the-knights-who-say-ni added the CLA not signed label May 15, 2018

bedevere-bot added the awaiting review label May 15, 2018

add news

72b53af

Mariatta requested changes May 16, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 16, 2018

mcscope added 2 commits May 15, 2018 21:11

CR from Mariatta

4be2015

Add to Ack

44114b3

bedevere-bot added awaiting change review and removed awaiting changes labels May 16, 2018

mcscope and others added 2 commits May 15, 2018 21:16

Spacing

ee76a11

Update News entry file, limit to 80 chars.

7129e9b

berkerpeksag requested changes May 16, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 16, 2018

mcscope added 4 commits May 15, 2018 22:16

code review by berkerpeksag

35442c5

Merge branch 'robotparser_site_maps' of github.com:mcscope/cpython in…

9b83c93

…to robotparser_site_maps Conflicts: Misc/NEWS.d/next/Library/2018-05-15-15-03-48.bpo-28612.E9dz39.rst

I think this will become 80 characters

1791c4e

This is more obviously 80 characters

97d8491

bedevere-bot added awaiting change review and removed awaiting changes labels May 16, 2018

berkerpeksag added 2 commits May 16, 2018 09:38

Update 2018-05-15-15-03-48.bpo-28612.E9dz39.rst

3e2aa65

Update test_robotparser.py

3e38354

berkerpeksag approved these changes May 16, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 16, 2018

Mariatta approved these changes May 16, 2018

View reviewed changes

ned-deily removed the CLA not signed label May 16, 2018

the-knights-who-say-ni added the CLA signed label May 16, 2018

ned-deily merged commit 5db5c06 into python:master May 16, 2018

bedevere-bot removed the awaiting merge label May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-21475: Support the Sitemap extension in robotparser #6883

bpo-21475: Support the Sitemap extension in robotparser #6883

mcscope commented May 15, 2018 •

edited

Loading

the-knights-who-say-ni commented May 15, 2018

mcscope commented May 15, 2018

Mariatta left a comment

Mariatta May 16, 2018

Mariatta May 16, 2018

bedevere-bot commented May 16, 2018

Mariatta commented May 16, 2018

mcscope commented May 16, 2018

bedevere-bot commented May 16, 2018

mcscope commented May 16, 2018

berkerpeksag May 16, 2018

berkerpeksag May 16, 2018

mcscope May 16, 2018

berkerpeksag May 16, 2018

berkerpeksag May 16, 2018

mcscope May 16, 2018

berkerpeksag May 16, 2018

berkerpeksag May 16, 2018

bedevere-bot commented May 16, 2018

mcscope commented May 16, 2018

mcscope commented May 16, 2018

bedevere-bot commented May 16, 2018

mcscope commented May 16, 2018

mcscope commented May 16, 2018

berkerpeksag commented May 16, 2018

berkerpeksag left a comment

ned-deily commented May 16, 2018

mcscope commented May 17, 2018

		@@ -0,0 +1,2 @@
		Added support for optional Site Map extension to urllib robotparser. Patch
		by Lady Red

bpo-21475: Support the Sitemap extension in robotparser #6883

bpo-21475: Support the Sitemap extension in robotparser #6883

Conversation

mcscope commented May 15, 2018 • edited Loading

the-knights-who-say-ni commented May 15, 2018

mcscope commented May 15, 2018

Mariatta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 16, 2018

Mariatta commented May 16, 2018

mcscope commented May 16, 2018

bedevere-bot commented May 16, 2018

mcscope commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 16, 2018

mcscope commented May 16, 2018

mcscope commented May 16, 2018

bedevere-bot commented May 16, 2018

mcscope commented May 16, 2018

mcscope commented May 16, 2018

berkerpeksag commented May 16, 2018

berkerpeksag left a comment

Choose a reason for hiding this comment

ned-deily commented May 16, 2018

mcscope commented May 17, 2018

mcscope commented May 15, 2018 •

edited

Loading