This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1033

pycompat: define operating system constants
ClosedPublic

Authored by quark on Oct 12 2017, 12:37 PM.

Details

Reviewers
spectral
Group Reviewers
hg-reviewers
Commits
rHGc0a6c19690ff: pycompat: define operating system constants
Summary

As suggested by Ryan in D1019, it's cleaner if we use defined constants
instead of osname == 'nt' everywhere.

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

quark created this revision.Oct 12 2017, 12:37 PM
spectral added a subscriber: spectral.Oct 12 2017, 2:18 PM
spectral added inline comments.
mercurial/pycompat.py
23

Since osname might be different (encoding or whatever) depending on python version, I think moving these after the 'if ispy3/else' block below and using something like:

isdarwin = sysplatform == 'darwin'
isposix = osname == 'posix'
iswindows = osname == 'nt'

would be better.

quark added inline comments.Oct 12 2017, 3:19 PM
mercurial/pycompat.py
23

For encoding, Python 2 and 3 are compatible if we use r prefixed strings.

The if condition is to normalize strings to bytes for Mercurial use. In other words, I think only types that need to be "normalized" should be put under the if condition.

These is* constants have a type of bool, which does not need to be normalized. I think it's better to avoid code duplication.

spectral accepted this revision.Oct 12 2017, 3:31 PM

[Accepted with a very mild concern; I'm fine with it not being addressed]

mercurial/pycompat.py
23

Ah, sorry, I didn't mean put it in the if blocks (duplicating it), I meant just put it at the end of the file once <thismodule>.osname and <thismodule>.sysplatform have been created in a python-version-aware fashion, not indented at all. (basically, move these three lines to the end of the file).

Mostly this is paranoia and a tiny extra layer of abstraction/indirection. I don't expect it to happen, but if sys.platform renames itself in some hypothetical future python 4.2, I expect we'll continue to have a pycompat.sysplatform, and then we shouldn't need to change these lines at all, if they're using that version-agnostic sysplatform.

I don't feel strongly about this though, and don't want to block you on this issue. I leave it up to you and the 2nd pass reviewers :)

quark added inline comments.Oct 12 2017, 3:39 PM
mercurial/pycompat.py
23

That makes sense. I will update the patch and the previous Jython one. Thanks!

quark updated this revision to Diff 2652.Oct 12 2017, 10:21 PM
spectral accepted this revision.Oct 12 2017, 10:23 PM
Closed by commit rHGc0a6c19690ff: pycompat: define operating system constants (authored by quark). · Explain WhyOct 13 2017, 10:06 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/pycompat.py (4 lines)

Diff 2682

mercurial/pycompat.py

import shlex import shlex
import sys import sys
ispy3 = (sys.version_info[0] >= 3) ispy3 = (sys.version_info[0] >= 3)
ispypy = (r'__pypy__' in sys.builtin_module_names) ispypy = (r'__pypy__' in sys.builtin_module_names)
if not ispy3: if not ispy3:
import cookielib import cookielib
import cPickle as pickle import cPickle as pickle
spectralUnsubmitted
Not Done

Since osname might be different (encoding or whatever) depending on python version, I think moving these after the 'if ispy3/else' block below and using something like:

isdarwin = sysplatform == 'darwin'
isposix = osname == 'posix'
iswindows = osname == 'nt'

would be better.

spectral: Since osname might be different (encoding or whatever) depending on python version, I think…
quarkAuthorUnsubmitted
Not Done

For encoding, Python 2 and 3 are compatible if we use r prefixed strings.

The if condition is to normalize strings to bytes for Mercurial use. In other words, I think only types that need to be "normalized" should be put under the if condition.

These is* constants have a type of bool, which does not need to be normalized. I think it's better to avoid code duplication.

quark: For encoding, Python 2 and 3 are compatible if we use `r` prefixed strings. The `if` condition…
spectralUnsubmitted
Not Done

Ah, sorry, I didn't mean put it in the if blocks (duplicating it), I meant just put it at the end of the file once <thismodule>.osname and <thismodule>.sysplatform have been created in a python-version-aware fashion, not indented at all. (basically, move these three lines to the end of the file).

Mostly this is paranoia and a tiny extra layer of abstraction/indirection. I don't expect it to happen, but if sys.platform renames itself in some hypothetical future python 4.2, I expect we'll continue to have a pycompat.sysplatform, and then we shouldn't need to change these lines at all, if they're using that version-agnostic sysplatform.

I don't feel strongly about this though, and don't want to block you on this issue. I leave it up to you and the 2nd pass reviewers :)

spectral: Ah, sorry, I didn't mean put it in the if blocks (duplicating it), I meant just put it at the…
quarkAuthorUnsubmitted
Not Done

That makes sense. I will update the patch and the previous Jython one. Thanks!

quark: That makes sense. I will update the patch and the previous Jython one. Thanks!
import httplib import httplib
import Queue as _queue import Queue as _queue
import SocketServer as socketserver import SocketServer as socketserver
import xmlrpclib import xmlrpclib
else: else:
import http.cookiejar as cookielib import http.cookiejar as cookielib
import http.client as httplib import http.client as httplib
import pickle import pickle
getcwd = os.getcwd getcwd = os.getcwd
sysexecutable = sys.executable sysexecutable = sys.executable
shlexsplit = shlex.split shlexsplit = shlex.split
stringio = cStringIO.StringIO stringio = cStringIO.StringIO
maplist = map maplist = map
rawinput = raw_input rawinput = raw_input
isjython = sysplatform.startswith('java') isjython = sysplatform.startswith('java')
isdarwin = sysplatform == 'darwin'
isposix = osname == 'posix'
iswindows = osname == 'nt'