bpo-35018: Sax parser provides no user access to lexical handlers. · Pull Request #10328 · python/cpython

ghost · 2018-11-05T02:36:05Z

This PR contains the functionality that is needed to fix bpo-6686 and bpo-9371, namely the LexicalHandler class which makes it easy to access XML comments and the document encoding. Those PR's depend on this issue.

https://bugs.python.org/issue35018

…ssue35018

test class definition.

tirkarthi · 2018-11-05T05:21:30Z

In the comment you have mentioned that this fixes issue9731 but I don't see anything in the PR related to https://bugs.python.org/issue9731 which is about adding ABCMeta.has_methods and tests for it. Please add in if I am missing something here.

Thanks!

ghost · 2018-11-06T02:00:08Z

It was my dyslexia - sorry. The correct number is [bpo-9371](https://bugs.python.org/issue9371).

…

On Sun, Nov 4, 2018 at 11:21 PM Xtreak ***@***.***> wrote: In the comment you have mentioned that this fixes issue9731 but I don't see anything in the PR related to https://bugs.python.org/issue9731 which is about adding ABCMeta.has_methods and tests for it. Please add in if I am missing something here. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10328 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWTNeAG7OkT11i3vnG1VXvIXqsJwkGYks5ur8rwgaJpZM4YNrPQ> .

tirkarthi · 2018-11-06T02:56:03Z

It's okay. It seems somehow the bot links the PR with ticket numbers in the PR comment. I don't know if it's intentional though. Thanks for the clarification.

serhiy-storchaka

Please edit the title and the description of this PR.

bedevere-bot · 2018-11-06T07:06:59Z

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.

ghost · 2018-11-07T14:32:50Z

I have made the requested changes; please review again .

bedevere-bot · 2018-11-07T14:32:53Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

I didn't actually review this.

serhiy-storchaka

Just few nitpicks.

serhiy-storchaka · 2018-11-08T06:56:53Z

                    SAXException, SAXReaderNotAvailable, SAXParseException
 import unittest
 from unittest import mock
+from Lib.pickle import FALSE


What relation does SAX have to pickle?

serhiy-storchaka · 2018-11-08T07:00:20Z

 version = '2.0beta'

-#============================================================================
+# ============================================================================


Unrelated change.

serhiy-storchaka · 2018-11-08T07:00:49Z

    Parser, the parser will call the method in your object to
    resolve all external entities. Note that DefaultHandler implements
-    this interface with the default behaviour."""
+    this interface with the default behavior."""


Unrelated change. Aren't both spellings correct?

serhiy-storchaka · 2018-11-08T07:02:45Z

@@ -0,0 +1,4 @@
+Add the LexicalHandler class that is present in other SAX XML
+implementations. The plumbing is already supported by Ptyhon so this simply
+adds the poecelain that makes it easy for users of the Python Sax parser to


"poecelain"?

Also I think it's Python not Ptyhon

ghost · 2018-11-08T16:28:24Z

I agree completely on almost all of the comments on this PR but I do have two questions. 1. Is there an accepted policy on English spelling usage for Python? My personal reaction is that spelling should be consistent across the entire corpus and since I felt that the combination of American English users and ESL users who tend to follow American English usage meant that American English was the best choice. I like to use spell checkers in my work because I am very prone to adjacent character typing errors and hence I automatically try to eliminate reported errors when I encounter them. But I fully accept that community policy should override individual preferences. 2 What is community policy regarding PEP8? I use autopep8 for all my Python work and in the case of a line such as "#####################" it reports "E265 block comment should start with '# '". I found that autopep8 accepts "# #####" but not "#####". I did sufficient work in xml.sax.handler.py that it seemed to me that it would be reasonable to eliminate all the PEP8 errors in it but again I will follow community policy.

…

On Thu, Nov 8, 2018 at 2:34 AM Xtreak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Misc/NEWS.d/next/Library/2018-11-03-21-21-32.[bpo-35018](https://bugs.python.org/issue35018).z1vnri.rst <#10328 (comment)>: > @@ -0,0 +1,4 @@ +Add the LexicalHandler class that is present in other SAX XML +implementations. The plumbing is already supported by Ptyhon so this simply +adds the poecelain that makes it easy for users of the Python Sax parser to Also I think it's Python not Ptyhon — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10328 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWTNVlhMgiFrgu8_V71aqsdRR6FkyeEks5us-yRgaJpZM4YNrPQ> .

tirkarthi · 2018-11-08T16:41:42Z

I am not a native speaker so I will defer to someone on the general tone of the documentation and the type of spelling.
Many of the code predates PEP8 and the general consensus is that unless you need to change it PEP8 related changes reduces the usefulness of git history that for example to track a comment you need to use git blame and changing the whitespace adds another step while reading git history for the line. Style guidelines are generally enforced for new code or code that you happen to change but making the old code PEP8 compliant around the change that you need to make is produces little gain and it's avoided.

ghost · 2018-11-08T17:01:11Z

I have made the requested changes; please review again .

bedevere-bot · 2018-11-08T17:01:15Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

ghost · 2018-11-08T18:11:08Z

@serhiy-storchaka Is it possible to mark issues bpo-6686 and bpo-9371 as depending on this issue bpo-35018. Both need the LexicalHandler to implement the fix and I don't have the authority to do this myself.

serhiy-storchaka · 2018-11-09T08:14:08Z

In any case this change is out of scope of this issue. Open a separate issue for fixing words in British spelling.
You should use PEP 8 for new code, and can use it for modified code, but left unrelated code untouched. And consistency with other code in the file can be more important than following some PEP 8 rules. PEP 8 explicitly allows this.

scoder

I agree with the request to exclude unrelated changes from this feature PR.

scoder · 2019-05-31T11:08:38Z

+    """This is implemented as a separate class since CDATA sections in XML
+    cannot appear within elements defined within a DTD. Hence this test does
+    not use a DTD."""
+    def __init__(self, *args, **kwargs):


Doing something in TestCase.__init__(), especially setting up test data, is a very unusual thing to do. Please use the normal set-up methods instead.

scoder · 2019-05-31T11:13:36Z

+    method with the property identifier
+    'http://xml.org/sax/handlers/LexicalHandler'."""
+
+    def xmlDecl(self, version, encoding, standalone):


This method is not part of the LexicalHandler interface.
http://www.saxproject.org/apidoc/org/xml/sax/ext/LexicalHandler.html

bedevere-bot · 2019-05-31T11:16:06Z

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.

csabella · 2019-10-20T18:50:33Z

@jgossage, please address the code review requests. Thanks!

csabella · 2019-12-14T01:03:35Z

I'm going to close this for now as the original PR author has not been active for awhile. This can be re-opened once work is ready to continue. Thanks!

JonathanGossage added 4 commits November 3, 2018 13:41

Add support for LexicalHandler class and documentation.

5a92d75

Merge branch 'master' of https://github.com/python/cpython.git into I…

a10380a

…ssue35018

Add testing CDATA handlers.

9a2d3d1

Merge branch 'master' of https://github.com/python/cpython.git into I…

2c8d73c

…ssue35018

bedevere-bot added the awaiting review label Nov 5, 2018

the-knights-who-say-ni added the CLA signed label Nov 5, 2018

JonathanGossage added 3 commits November 4, 2018 20:49

Fix whitespace problem.

69ae816

Fix CDATAHandlerTest - was not tested because of missing super class in

d7d4d30

test class definition.

Fix long-standing typo in xml.sax.handler.rst.

db60cb5

serhiy-storchaka requested changes Nov 6, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 6, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 7, 2018

zware changed the title ~~Issue35018~~ bpo-35018: Sax parser provides no user access to lexical handlers. Nov 7, 2018

serhiy-storchaka added the type-feature A feature request or enhancement label Nov 8, 2018

serhiy-storchaka previously approved these changes Nov 8, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 8, 2018

serhiy-storchaka reviewed Nov 8, 2018

View reviewed changes

Fix areas suggested by review comments.

d8dc65e

bedevere-bot removed the awaiting merge label Nov 8, 2018

bedevere-bot added the awaiting change review label Nov 8, 2018

csabella requested a review from scoder May 30, 2019 20:01

scoder requested changes May 31, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 31, 2019

csabella closed this Dec 14, 2019

Uh oh!

Conversation

ghost commented Nov 5, 2018 • edited by zware Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Nov 5, 2018

Uh oh!

ghost commented Nov 6, 2018 via email • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Nov 6, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 6, 2018

Uh oh!

ghost commented Nov 7, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Nov 7, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

tirkarthi Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 8, 2018 via email • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Nov 8, 2018

Uh oh!

ghost commented Nov 8, 2018

Uh oh!

bedevere-bot commented Nov 8, 2018

Uh oh!

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

Uh oh!

Uh oh!

serhiy-storchaka commented Nov 9, 2018

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder May 31, 2019

Choose a reason for hiding this comment

Uh oh!

scoder May 31, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 31, 2019

Uh oh!

csabella commented Oct 20, 2019

Uh oh!

csabella commented Dec 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ghost commented Nov 5, 2018 •

edited by zware

Loading

ghost commented Nov 6, 2018 via email •

edited by bedevere-bot

Loading

ghost commented Nov 7, 2018 •

edited by ghost

Loading

ghost commented Nov 8, 2018 via email •

edited by bedevere-bot

Loading

ghost commented Nov 8, 2018 •

edited by bedevere-bot

Loading