| Submitter | phabricator |
|---|---|
| Date | Oct. 12, 2017, 4:37 p.m. |
| Message ID | <differential-rev-PHID-DREV-e2hy364gw2bsesui2wya-req@phab.mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/24795/ |
| State | Superseded |
| Headers | show |
Comments
spectral added inline comments. INLINE COMMENTS > pycompat.py:24 > +isposix = os.name == r'posix' > +iswindows = os.name == r'nt' > + 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1033 To: quark, #hg-reviewers Cc: spectral, mercurial-devel
quark added inline comments. INLINE COMMENTS > spectral wrote in pycompat.py:24 > 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. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1033 To: quark, #hg-reviewers Cc: spectral, mercurial-devel
spectral accepted this revision. spectral added a comment. [Accepted with a very mild concern; I'm fine with it not being addressed] INLINE COMMENTS > quark wrote in pycompat.py:24 > 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. 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 :) REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1033 To: quark, #hg-reviewers, spectral Cc: spectral, mercurial-devel
quark added inline comments. INLINE COMMENTS > spectral wrote in pycompat.py:24 > 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 :) That makes sense. I will update the patch and the previous Jython one. Thanks! REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1033 To: quark, #hg-reviewers, spectral Cc: spectral, mercurial-devel
Patch
diff --git a/mercurial/pycompat.py b/mercurial/pycompat.py --- a/mercurial/pycompat.py +++ b/mercurial/pycompat.py @@ -19,6 +19,10 @@ ispypy = (r'__pypy__' in sys.builtin_module_names) isjython = sys.platform.startswith(r'java') +isdarwin = sys.platform == r'darwin' +isposix = os.name == r'posix' +iswindows = os.name == r'nt' + if not ispy3: import cookielib import cPickle as pickle