bpo-21475: Support the Sitemap extension in robotparser by mcscope · Pull Request #6883 · python/cpython

mcscope · May 15, 2018

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 · May 15, 2018

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 · May 15, 2018

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 · May 16, 2018

+
+      .. 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 · May 16, 2018

@@ -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 · May 16, 2018

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 · May 16, 2018

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 · May 16, 2018

I have made the requested changes; please review again.

bedevere-bot · May 16, 2018

Thanks for making the requested changes!

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

mcscope · May 16, 2018

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

berkerpeksag · May 16, 2018

+    """
+    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 · May 16, 2018

            # 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 · May 16, 2018


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


Please don't make unrelated cosmetic changes.

berkerpeksag · May 16, 2018

        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 · May 16, 2018

@@ -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 · May 16, 2018

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 · May 16, 2018

@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 · May 16, 2018

I have made the requested changes; please review again.

bedevere-bot · May 16, 2018

Thanks for making the requested changes!

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

mcscope · May 16, 2018

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

mcscope · May 16, 2018

Successfully Tested! The news link works

berkerpeksag · May 16, 2018

@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 · May 16, 2018

Congratulations on your first cpython PR, @mcscope!

mcscope · May 17, 2018

@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

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

Search code, repositories, users, issues, pull requests...

Uh oh!

Uh oh!

Conversation

mcscope commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

mcscope commented May 15, 2018

Uh oh!

Mariatta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 16, 2018

Uh oh!

Mariatta commented May 16, 2018

Uh oh!

mcscope commented May 16, 2018

Uh oh!

bedevere-bot commented May 16, 2018

Uh oh!

mcscope commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 16, 2018

Uh oh!

mcscope commented May 16, 2018

Uh oh!

mcscope commented May 16, 2018

Uh oh!

bedevere-bot commented May 16, 2018

Uh oh!

mcscope commented May 16, 2018

Uh oh!

mcscope commented May 16, 2018

Uh oh!

berkerpeksag commented May 16, 2018

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

ned-deily commented May 16, 2018

Uh oh!

mcscope commented May 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mcscope commented May 15, 2018 •

edited

Loading