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

bpo-41842: Add a unregister function in _codecs module #22360

Merged
merged 15 commits into from
Sep 28, 2020

Conversation

shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Sep 22, 2020

Add a _codecs.unregister() in _codecs module to unregister search function.

https://bugs.python.org/issue41842

@shihai1991 shihai1991 changed the title Add a unregister function in _codecs module [WIP] Add a unregister function in _codecs module Sep 22, 2020
@shihai1991 shihai1991 changed the title [WIP] Add a unregister function in _codecs module bpo-39337: Add a unregister function in _codecs module Sep 22, 2020
@shihai1991
Copy link
Member Author

shihai1991 commented Sep 22, 2020

Pls take a look if you have free times, thanks. @vstinner @serhiy-storchaka

Copy link
Member

@vstinner vstinner left a 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 an entry in the Doc/whatsnew/3.10.rst document? You have to add a news "codecs" section near:
https://docs.python.org/dev/whatsnew/3.10.html#improved-modules

Doc/library/codecs.rst Outdated Show resolved Hide resolved
Doc/library/codecs.rst Outdated Show resolved Hide resolved
Modules/_codecsmodule.c Outdated Show resolved Hide resolved
Python/codecs.c Outdated Show resolved Hide resolved
Python/codecs.c Outdated Show resolved Hide resolved
Python/codecs.c Outdated Show resolved Hide resolved
Python/codecs.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

It would be nice to use this new function to remove an old side effect of test_codecs: codecs.register(_get_test_codec) registers a search function which is never unregistered.

Maybe ExceptionChainingTest.setUp() method should register the search function and use self.addCleanup(codecs.unregister, _get_test_codec)?

@vstinner
Copy link
Member

See this comment of test_codecs.py:

        # There's no way to unregister a codec search function, so we just
        # ensure we render this one fairly harmless after the test
        # case finishes by using the test case repr as the codec name
        # The codecs module normalizes codec names, although this doesn't
        # appear to be formally documented...
        # We also make sure we use a truly unique id for the custom codec
        # to avoid issues with the codec cache when running these tests
        # multiple times (e.g. when hunting for refleaks)

And https://bugs.python.org/issue22166

@vstinner
Copy link
Member

Old email requesting codecs.unregister() function: https://mail.python.org/pipermail/python-dev/2011-September/113588.html

@vstinner
Copy link
Member

https://bugs.python.org/issue39337 is not directly related to codecs.unregister().

@shihai1991: I created https://bugs.python.org/issue41842 "Add codecs.unregister() to unregister a codec search function": please use this bpo for your PR.

@shihai1991
Copy link
Member Author

Maybe ExceptionChainingTest.setUp() method should register the search function and use self.addCleanup(codecs.unregister, _get_test_codec)?

+1, it is in my TODO list :)

@vstinner
Copy link
Member

Updating test_codecs can be done in a second following PR if you prefer.

@shihai1991 shihai1991 changed the title bpo-39337: Add a unregister function in _codecs module bpo-41842: Add a unregister function in _codecs module Sep 23, 2020
@shihai1991
Copy link
Member Author

https://bugs.python.org/issue39337 is not directly related to codecs.unregister().

@shihai1991: I created https://bugs.python.org/issue41842 "Add codecs.unregister() to unregister a codec search function": please use this bpo for your PR.

copy that, this bpo have been used in this PR.

Doc/library/codecs.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Include/codecs.h Outdated Show resolved Hide resolved
Lib/test/test_codecs.py Outdated Show resolved Hide resolved
Modules/_codecsmodule.c Outdated Show resolved Hide resolved
Modules/_codecsmodule.c Outdated Show resolved Hide resolved
Python/codecs.c Show resolved Hide resolved
Include/codecs.h Show resolved Hide resolved
Include/codecs.h Show resolved Hide resolved
Lib/test/test_codecs.py Outdated Show resolved Hide resolved
Python/codecs.c Outdated Show resolved Hide resolved
Doc/c-api/codec.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Include/codecs.h Outdated Show resolved Hide resolved
Include/codecs.h Outdated Show resolved Hide resolved
Lib/test/test_codecs.py Show resolved Hide resolved
Lib/test/test_codecs.py Outdated Show resolved Hide resolved
* origin/master: (27 commits)
  bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388)
  bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387)
  bpo-41833: threading.Thread now uses the target name (pythonGH-22357)
  bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633)
  bpo-33822: Update IDLE section of What's New 3.8 (pythonGH-22383)
  bpo-41844: Add IDLE section to What's New 3.9 (GN-22382)
  bpo-41841: Prepare IDLE News for 3.10 (pythonGH-22379)
  bpo-37779 : Add information about the overriding behavior of ConfigParser.read (pythonGH-15177)
  bpo-40170: Use inline _PyType_HasFeature() function (pythonGH-22375)
  bpo-40941: Fix stackdepth compiler warnings (pythonGH-22377)
  bpo-40941: Fix fold_tuple_on_constants() compiler warnings (pythonGH-22378)
  bpo-40521: Fix PyUnicode_InternInPlace() (pythonGH-22376)
  bpo-41834: Remove _Py_CheckRecursionLimit variable (pythonGH-22359)
  bpo-1635741, unicodedata: add ucd_type parameter to UCD_Check() macro (pythonGH-22328)
  bpo-1635741: Port _lsprof extension to multi-phase init (PEP 489) (pythonGH-22220)
  bpo-41513: Improve order of adding fractional values. Improve variable names. (pythonGH-22368)
  bpo-41816: `StrEnum.__str__` is `str.__str__` (pythonGH-22362)
  bpo-35764: Rewrite the IDLE Calltips doc section  (pythonGH-22363)
  bpo-41810: Reintroduce `types.EllipsisType`, `.NoneType` & `.NotImplementedType` (pythonGH-22336)
  bpo-41602: raise SIGINT exit code on KeyboardInterrupt from pymain_run_module (python#21956)
  ...
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just requested a bunch of minor edits.

I leave the PR open a few days if @malemburg wants to have a look.

cc @serhiy-storchaka

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Lib/test/test_codecs.py Show resolved Hide resolved
Lib/test/test_codecs.py Show resolved Hide resolved
Python/codecs.c Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Python/codecs.c Outdated
return 0;
}

Py_ssize_t n = PyList_Size(codec_search_path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the macro PyList_GET_SIZE() instead of PyList_Size(), as codec_search_path is guaranteed to be a list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's a list right now, its type can change tomorrow.

@shihai1991: If you modify the code to use fast macros, please add the following assertion before using it:

assert(PyList_CheckExact(interp->codec_search_path));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vstinner Copy that, I have already updated it.
But I have a question. assert(PyList_Check(op) in _PyList_CAST() is not good enough?

Python/codecs.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

Merged, thank you @shihai1991!

@shihai1991
Copy link
Member Author

shihai1991 commented Sep 29, 2020

Thanks for everyone's review: ).

shihai1991 added a commit to shihai1991/cpython that referenced this pull request Sep 29, 2020
* origin/master: (113 commits)
  bpo-41773: Raise exception for non-finite weights in random.choices().  (pythonGH-22441)
  bpo-41873: Add vectorcall for float() (pythonGH-22432)
  bpo-41861: Convert _sqlite3 PrepareProtocolType to heap type (pythonGH-22428)
  bpo-41842: Add codecs.unregister() function (pythonGH-22360)
  bpo-41875: Use __builtin_unreachable when possible (pythonGH-22433)
  bpo-40105: ZipFile truncate in append mode with shorter comment (pythonGH-19337)
  bpo-41870: Use PEP 590 vectorcall to speed up bool()  (pythonGH-22427)
  [doc] Leverage the fact that the actual types can now be indexed for typing (pythonGH-22340)
  bpo-41861: Convert _sqlite3 cache and node static types to heap types (pythonGH-22417)
  bpo-41858: Clarify line in optparse doc (pythonGH-22407)
  Revert "Fix all Python Cookbook links (python#22205)" (pythonGH-22424)
  bpo-1635741: Port _bisect module to multi-phase init (pythonGH-22415)
  bpo-41428: Fix compiler warning in unionobject.c (pythonGH-22416)
  Fix logging error message (pythonGH-22410)
  bpo-39934: Account for control blocks in 'except' in compiler. (pythonGH-22395)
  bpo-41775: Make 'IDLE Shell' the shell title  (python#22399)
  bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388)
  bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387)
  bpo-41833: threading.Thread now uses the target name (pythonGH-22357)
  bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633)
  ...
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
Add codecs.unregister() and PyCodec_Unregister() functions
to unregister a codec search function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants