-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
bpo-33865: Add alias for encodings #7705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi! Thank you for the PR. Could you add a NEWS entry and update the |
@pablogsal I have added a NEWS entry with blurb tool and updated the docs. Thanks. |
@@ -0,0 +1 @@ | |||
Add aliases for encodings. Patch by Karthikeyan Singaravelan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say: "for code page encodings (ex: 874 for cp874)."
@@ -1024,9 +1024,9 @@ particular, the following variants typically exist: | |||
| cp500 | EBCDIC-CP-BE, EBCDIC-CP-CH, | Western Europe | | |||
| | IBM500 | | | |||
+-----------------+--------------------------------+--------------------------------+ | |||
| cp720 | | Arabic | | |||
| cp720 | 720 | Arabic | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add ".. versionchanged:: 3.7" markup after the table to mention that you added aliases to cp720, cp737, ... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner You mean "".. versionchanged:: 3.8", no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... right :-) Sorry our 3.7 release manager!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that. Fixed with 27d9380 . Thanks.
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 |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
But @serhiy-storchaka proposed a different approach: https://bugs.python.org/issue33865#msg319617
@serhiy-storchaka: Are you ok with this change? I'm not 100% comfortable with your idea of modified search function. I prefer an explicit list of aliases.
@vstinner There is a discussion about this in python-ideas and there is no clear decision on whether to assign numerical aliases or it's a problem that needs to be fixed in OP's machine. OP confirmed that patch related changes fixed the issue but from the python-idea's discussion I could see this to be merged only as a last resort. Maybe we can wait for some response from OP about his system configuration or a concrete decision from the discussion. Ref : https://groups.google.com/d/msg/python-ideas/Ny1RN9wY0cI/ug6MnnEUAwAJ |
As per the discussion on the tracker this seems to be an issue with Anaconda. I am closing this PR. Thanks much @methane for the detailed analysis . Ref : https://bugs.python.org/issue33865#msg320426 |
Added alias for below :
Ref comment : https://bugs.python.org/msg319590
Ref for alias : https://docs.python.org/3.8/library/codecs.html#standard-encodings
Please let me know if I need to auto-generate any files or missing something.
Thanks
https://bugs.python.org/issue33865