Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(120)

#20035: Suppress 'os.environ was modified' warning on Tcl/Tk tests

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by zachary.ware
Modified:
4 years, 10 months ago
Reviewers:
storchaka
CC:
terry.reedy, pmoore, haypo, tim.golden, r.david.murray, BreamoreBoy, devnull_psf.upfronthosting.co.za, Zach Ware, storchaka, steve.dower
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Total comments: 8

Patch Set 7 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/test_tcl.py View 1 2 3 4 5 6 1 chunk +1 line, -3 lines 0 comments Download
Lib/test/test_tk.py View 1 2 3 4 5 6 1 chunk +0 lines, -3 lines 0 comments Download
Lib/test/test_ttk_guionly.py View 1 2 3 4 5 6 1 chunk +1 line, -3 lines 0 comments Download
Lib/test/test_ttk_textonly.py View 1 2 3 4 5 6 1 chunk +0 lines, -3 lines 0 comments Download
Lib/tkinter/_fix.py View 1 2 3 4 5 6 1 chunk +0 lines, -78 lines 0 comments Download
Lib/tkinter/__init__.py View 1 2 3 4 5 6 1 chunk +0 lines, -3 lines 0 comments Download
Modules/_tkinter.c View 1 2 3 4 5 6 3 chunks +118 lines, -1 line 0 comments Download
PCbuild/_tkinter.vcxproj View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
Tools/buildbot/test-amd64.bat View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
Tools/buildbot/test.bat View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 5
storchaka_gmail.com
http://bugs.python.org/review/20035/diff/10397/Lib/test/test_tcl.py File Lib/test/test_tcl.py (left): http://bugs.python.org/review/20035/diff/10397/Lib/test/test_tcl.py#oldcode13 Lib/test/test_tcl.py:13: support.import_fresh_module('tkinter') I think support.import_fresh_module('tkinter._fix') should be added in setUpModule(). ...
6 years, 3 months ago #1
Zach Ware
http://bugs.python.org/review/20035/diff/10397/Lib/test/test_tcl.py File Lib/test/test_tcl.py (left): http://bugs.python.org/review/20035/diff/10397/Lib/test/test_tcl.py#oldcode13 Lib/test/test_tcl.py:13: support.import_fresh_module('tkinter') On 2013/12/20 18:59:57, storchaka wrote: > I think ...
6 years, 3 months ago #2
storchaka_gmail.com
Sorry for the delay. http://bugs.python.org/review/20035/diff/10397/Lib/test/test_tcl.py File Lib/test/test_tcl.py (left): http://bugs.python.org/review/20035/diff/10397/Lib/test/test_tcl.py#oldcode13 Lib/test/test_tcl.py:13: support.import_fresh_module('tkinter') On 2013/12/20 19:24:14, Zach ...
6 years, 2 months ago #3
storchaka_gmail.com
https://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c File Modules/_tkinter.c (right): https://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c#newcode121 Modules/_tkinter.c:121: tcl_library_path = PyUnicode_Concat(prefix, PyUnicode_FromString and PyUnicode_Concat can fail. Their ...
4 years, 10 months ago #4
Zach Ware
4 years, 10 months ago #5
Thank you very much for the review!

http://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c
File Modules/_tkinter.c (right):

http://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c#newcode121
Modules/_tkinter.c:121: tcl_library_path = PyUnicode_Concat(prefix,
On 2015/05/17 07:17:24, storchaka wrote:
> PyUnicode_FromString and PyUnicode_Concat can fail. Their results should be
> tested for NULL.

Done.

http://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c#newcode125
Modules/_tkinter.c:125: if (_Py_stat(tcl_library_path, &stat_buf) == -1) {
On 2015/05/17 07:17:24, storchaka wrote:
> _Py_stat can return -2.

Done.

http://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c#newcode130
Modules/_tkinter.c:130: tcl_library_path = PyUnicode_FromString(
On 2015/05/17 07:17:24, storchaka wrote:
> The same as above.

Done.

http://bugs.python.org/review/20035/diff/14867/Modules/_tkinter.c#newcode735
Modules/_tkinter.c:735: if (str_path != NULL) {
On 2015/05/17 07:17:24, storchaka wrote:
> If str_path == NULL, the exception can be set. If the exception is set, the
> constructor should fail.

I was thinking that at this point, _get_tcl_lib_path() would have already been
called, but it would be possible to have TCL_LIBRARY set while initializing the
module, and unset before Tkapp is initialized.

Fixed.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+