This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2864

contrib: open a hole in the open().read() ban for open().close()
ClosedPublic

Authored by durin42 on Mar 14 2018, 3:53 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG35d814fe2f30: contrib: open a hole in the open().read() ban for open().close()
rHGe1899cb031c4: contrib: open a hole in the open().read() ban for open().close()
rHG1190449c807d: contrib: open a hole in the open().read() ban for open().close()
Summary

It turns out open().close() is both fine and something we occasionally
do to verify something can be written. The few cases in the codebase
were getting missed due to a regular expression bug (which I discussed
in my previous change), but since I'm about to fix the bug, I need to
fix the patterns.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Mar 14 2018, 3:53 PM
pulkit accepted this revision.Mar 15 2018, 8:27 AM
Closed by commit rHG1190449c807d: contrib: open a hole in the open().read() ban for open().close() (authored by durin42). · Explain WhyMar 15 2018, 8:53 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcontrib/check-code.py (4 lines)

Diff 7059

contrib/check-code.py

(r'opener\([^)]*\).read\(', (r'opener\([^)]*\).read\(',
"use opener.read() instead"), "use opener.read() instead"),
(r'opener\([^)]*\).write\(', (r'opener\([^)]*\).write\(',
"use opener.write() instead"), "use opener.write() instead"),
(r'[\s\(](open|file)\([^)]*\)\.read\(', (r'[\s\(](open|file)\([^)]*\)\.read\(',
"use util.readfile() instead"), "use util.readfile() instead"),
(r'[\s\(](open|file)\([^)]*\)\.write\(', (r'[\s\(](open|file)\([^)]*\)\.write\(',
"use util.writefile() instead"), "use util.writefile() instead"),
(r'^[\s\(]*(open(er)?|file)\([^)]*\)', (r'^[\s\(]*(open(er)?|file)\([^)]*\)(?!\.close\(\))',
"always assign an opened file to a variable, and close it afterwards"), "always assign an opened file to a variable, and close it afterwards"),
(r'[\s\(](open|file)\([^)]*\)\.', (r'[\s\(](open|file)\([^)]*\)\.(?!close\(\))',
"always assign an opened file to a variable, and close it afterwards"), "always assign an opened file to a variable, and close it afterwards"),
(r'(?i)descend[e]nt', "the proper spelling is descendAnt"), (r'(?i)descend[e]nt', "the proper spelling is descendAnt"),
(r'\.debug\(\_', "don't mark debug messages for translation"), (r'\.debug\(\_', "don't mark debug messages for translation"),
(r'\.strip\(\)\.split\(\)', "no need to strip before splitting"), (r'\.strip\(\)\.split\(\)', "no need to strip before splitting"),
(r'^\s*except\s*:', "naked except clause", r'#.*re-raises'), (r'^\s*except\s*:', "naked except clause", r'#.*re-raises'),
(r'^\s*except\s([^\(,]+|\([^\)]+\))\s*,', (r'^\s*except\s([^\(,]+|\([^\)]+\))\s*,',
'legacy exception syntax; use "as" instead of ","'), 'legacy exception syntax; use "as" instead of ","'),
(r':\n( )*( ){1,3}[^ ]', "must indent 4 spaces"), (r':\n( )*( ){1,3}[^ ]', "must indent 4 spaces"),