Skip to content
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

Remove redundant coding cookies from 3.x stdlib #53554

Closed
abalkin opened this issue Jul 19, 2010 · 19 comments
Closed

Remove redundant coding cookies from 3.x stdlib #53554

abalkin opened this issue Jul 19, 2010 · 19 comments
Assignees
Labels
stdlib Python modules in the Lib dir tests Tests in the Lib/test dir

Comments

@abalkin
Copy link
Member

abalkin commented Jul 19, 2010

BPO 9308
Nosy @mdickinson, @abalkin, @merwok, @bitdancer
Files
  • issue9308.diff
  • issue9308a.diff
  • issue9308b.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/abalkin'
    closed_at = <Date 2010-10-15.16:29:28.770>
    created_at = <Date 2010-07-19.18:57:07.460>
    labels = ['tests', 'library']
    title = 'Remove redundant coding cookies from 3.x stdlib'
    updated_at = <Date 2010-10-16.20:15:48.355>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2010-10-16.20:15:48.355>
    actor = 'belopolsky'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2010-10-15.16:29:28.770>
    closer = 'belopolsky'
    components = ['Library (Lib)', 'Tests']
    creation = <Date 2010-07-19.18:57:07.460>
    creator = 'belopolsky'
    dependencies = []
    files = ['18067', '18093', '19244']
    hgrepos = []
    issue_num = 9308
    keywords = ['patch']
    message_count = 19.0
    messages = ['110798', '110804', '110805', '110807', '110828', '110830', '110835', '110839', '110840', '110853', '110860', '110907', '110921', '110922', '111203', '118785', '118797', '118878', '118889']
    nosy_count = 6.0
    nosy_names = ['niemeyer', 'astrand', 'mark.dickinson', 'belopolsky', 'eric.araujo', 'r.david.murray']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue9308'
    versions = ['Python 3.2']

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 19, 2010

    On Mon, Jul 19, 2010 at 2:45 AM, Guido van Rossum <guido@python.org> wrote:

    Sounds like a good idea to try to remove redundant cookies *and* to
    remove most occasional use of non-ASCII characters outside comments
    (except for unittests specifically trying to test Unicode features).
    Personally I would use \xXX escapes instead of spelling out the
    characters in shlex.py, for example.

    Both with or without the coding cookies, many ways of displaying text
    files garble characters outside the ASCII range, so it's better to
    stick to ASCII as much as possible.

    --Guido

    On Mon, Jul 19, 2010 at 1:21 AM, Alexander Belopolsky
    <alexander.belopolsky@gmail.com> wrote:
    > I was looking at the inspect module and noticed that it's source
    > starts with "# -- coding: iso-8859-1 --".   I have checked and there
    > are no non-ascii characters in the file.   There are several other
    > modules that still use the cookie:
    >
    > Lib/ast.py:# -- coding: utf-8 --
    > Lib/getopt.py:# -- coding: utf-8 --
    > Lib/inspect.py:# -- coding: iso-8859-1 --
    > Lib/pydoc.py:# -- coding: latin-1 --
    > Lib/shlex.py:# -- coding: iso-8859-1 --
    > Lib/encodings/punycode.py:# -- coding: utf-8 --
    > Lib/msilib/init.py:# -- coding: utf-8 --
    > Lib/sqlite3/init.py:#-- coding: ISO-8859-1 --
    > Lib/sqlite3/dbapi2.py:#-- coding: ISO-8859-1 --
    > Lib/test/bad_coding.py:# -- coding: uft-8 --
    > Lib/test/badsyntax_3131.py:# -- coding: utf-8 --
    >
    > I understand that coding: utf-8 is strictly redundant in 3.x.  There
    > are cases such as Lib/shlex.py where using encoding other than utf-8
    > is justified.  (See
    > http://svn.python.org/view?view=rev&revision=82560).  What are the
    > guidelines for other cases?  Should redundant cookies be removed?
    > Since not all editors respect the  -*- cookie, I think the answer
    > should be "yes" particularly when the cookie is setting encoding other
    > than utf-8.
    > _______________________________________________
    > Python-Dev mailing list
    > Python-Dev@python.org
    > http://mail.python.org/mailman/listinfo/python-dev
    > Unsubscribe: http://mail.python.org/mailman/options/python-dev/guido%40python.org
    >

    --
    --Guido van Rossum (python.org/~guido)

    @abalkin abalkin self-assigned this Jul 19, 2010
    @abalkin abalkin added the stdlib Python modules in the Lib dir label Jul 19, 2010
    @merwok merwok added the easy label Jul 19, 2010
    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 19, 2010

    In the attached patch, I've removed encoding cookies in all files listed in the message above except

    > Lib/test/bad_coding.py:# -- coding: uft-8 --
    > Lib/test/badsyntax_3131.py:# -- coding: utf-8 --
    > Lib/shlex.py:# -- coding: iso-8859-1 --

    The bad_coding contains bad encoding cookie which is being tested, in badsyntax_3131, the cookie could be removed, but I think it clarifies the meaning of the file. It can probably be replaced with a comment explaining what is being tested.

    The shlex.py was reverted in r82560, so I am not touching it, but I suspect that the logic used to build wordchars is not correct on systems supporting unicode.

    I am adding Peter Åstrand to "nosy" because his name was miscoded in Lib/getopt.py and I fixed it by copying from Misc/ACKS.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 19, 2010

    It turns out that test_imp tests that pydoc.py is in iso-8859-1. This is not right. The test should use a dedicated file under Lib/test for that.

    @merwok
    Copy link
    Member

    merwok commented Jul 19, 2010

    This is not easy after all, kudos for taking care of that.

    The shlex fix was reverted IIRC because Mark was unsure of the fix, since our diff tools try to be too clever and decode files, not showing us the bytes differences. I don’t trust my editor either, nor the file command (which says very funny things about Python files). I only trust Python’s open function to give me the exact bytes, which I can compare with 'æ'.encode('utf-8') and 'æ'.encode('latin1') to make up my mind.

    In this case, I would trust the left-hand side of the diff seen in r82560 as a definition of what those characters are supposed to be, since I don’t have POSIX handy <wink>, and add the \x escapes for those characters (per Guido’s recommendation—I regret we’ve switched to UTF-8 but can’t use it).

    @merwok merwok removed the easy label Jul 19, 2010
    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 19, 2010

    On Mon, Jul 19, 2010 at 4:33 PM, Éric Araujo <report@bugs.python.org> wrote:
    ..

    The shlex fix was reverted IIRC because Mark was unsure of the fix, since our diff tools
    try to be too clever and decode files, not showing us the bytes differences.

    Well, it is not clear what wordchars should be in shlex. I would
    think (c in wordchar) should be the same as (c.isalnum() or c == '_'),
    but there are only 62 characters added added in in posix mode:

    62

    while according to str.isalnum(), three are 71 alphanumeric characters
    at code points between 128 and 255:

    71

    I don't know what POSIX definition of word character is.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 19, 2010

    Tracker eating python prompt prefixed lines is truly annoying. Here are the missing lines:

    >>> len(shlex(posix=True).wordchars) - len(shlex().wordchars)
    62
    >>> len([chr(i) for i in range(128, 256) if chr(i).isalnum()])
    71

    @merwok
    Copy link
    Member

    merwok commented Jul 19, 2010

    Thank you for explaining that, I had a suspicion I had not understood “the logic used to build wordchars is not correct”. So our options here are to ignore the issue and keep statu quo, remove this strange POSIX wordchars concept, find some POSIX docs, or contact esr directly. I’m tentatively adding him to nosy, hoping he’ll see the email and come here enlighten us. Eric, I hope this doesn’t annoy you.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Jul 19, 2010

    Changes by Éric Araujo <merwok@netwok.org>:
    ----------
    nosy: +esr

    I don't think posix mode was added by ESR, but I cannot check ATM.

    @merwok
    Copy link
    Member

    merwok commented Jul 19, 2010

    I shouldn’t edit bugs when tired, sorry for the noise. hg log tells me that those extra characters have been added in r32284 (2003) following bug bpo-722686 by Gustavo Niemeyer following a discussion on “the mailing list”. Adjusting nosy.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 20, 2010

    I wonder whether 3.x shlex working with unicode files is just incidental to 3.x string model and is not really correct. I think bpo-1170 is a better place to have this discussion. I'll add a comment there.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 20, 2010

    I am attaching a new patch, issue9308a.diff which fixes test_imp by introducing a new package, test.encoded_modules which currently contains a module encoded with iso-8859-1 and another with a somewhat more interesting encoding, koi8-r. I think it would be interesting to add one or two multi-byte encodings to the mix. The test_import_encoded_module test case probably does not belong in test_imp and should be moved to test_import or test_importlib, but I would like to see the reaction to the idea of introducing test.encoded_modules before investing more time into polishing the tests.

    @merwok
    Copy link
    Member

    merwok commented Jul 20, 2010

    Could you add comments in encoded_modules/init.py? I don’t understand it, so I can’t make a useful comment.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 20, 2010

    Could you add comments in encoded_modules/init.py?

    Please see updated issue9308a.diff.

    @merwok
    Copy link
    Member

    merwok commented Jul 20, 2010

    I think this new test is a good idea.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 22, 2010

    Is anyone interested in reviewing this patch before it is committed? Since there are no user-visible changes and the only non-trivial change simply adds new tests, I think this can go in. Any refinements can be done later.

    @abalkin abalkin added the tests Tests in the Lib/test dir label Jul 22, 2010
    @abalkin
    Copy link
    Member Author

    abalkin commented Oct 15, 2010

    I am attaching an updated patch, issue9308b.diff. Compared to the "a" patch, I added test/encoded_modules to the makefile so that it gets installed and removed cookies from some more test and Tools files.

    @abalkin
    Copy link
    Member Author

    abalkin commented Oct 15, 2010

    Committed in r85537.

    @abalkin abalkin closed this as completed Oct 15, 2010
    @bitdancer
    Copy link
    Member

    I yhink you need to add an svnignore property to that directory for __pycache__.

    @abalkin
    Copy link
    Member Author

    abalkin commented Oct 16, 2010

    On Sat, Oct 16, 2010 at 1:34 PM, R. David Murray <report@bugs.python.org> wrote:
    ..

    I yhink you need to add an svnignore property to that directory for __pycache__.

    r85576 (I hope I got it right.)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants