bpo-35018: Sax parser provides no user access to lexical handlers. · Pull Request #10328 · python/cpython (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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!
Author
ghost commented
via email
• Loading
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please edit the title and the description of this PR.
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.
I have made the requested changes; please review again .
Thanks for making the requested changes!
@serhiy-storchaka: please review the changes made to this pull request.
Issue35018 bpo-35018: Sax parser provides no user access to lexical handlers.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just few nitpicks.
| @@ -5,6 +5,7 @@ |
|---|
| SAXException, SAXReaderNotAvailable, SAXParseException |
| import unittest |
| from unittest import mock |
| from Lib.pickle import FALSE |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What relation does SAX have to pickle?
| @@ -11,11 +11,12 @@ |
|---|
| version = '2.0beta' |
| #============================================================================ |
| # ============================================================================ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change.
| @@ -225,7 +226,7 @@ class EntityResolver: |
|---|
| implementing this interface, then register the object with your |
| 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.""" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change. Aren't both spellings correct?
| @@ -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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"poecelain"?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think it's Python not Ptyhon
Author
ghost commented
via email
• Loading
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.
- 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.
I have made the requested changes; please review again .
Thanks for making the requested changes!
@serhiy-storchaka: please review the changes made to this pull request.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the request to exclude unrelated changes from this feature PR.
| """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): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| method with the property identifier |
|---|
| 'http://xml.org/sax/handlers/LexicalHandler'.""" |
| def xmlDecl(self, version, encoding, standalone): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@jgossage, please address the code review requests. Thanks!
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!