Patchwork D1033: pycompat: define operating system constants

login
register
mail settings
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

phabricator - Oct. 12, 2017, 4:37 p.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As suggested by Ryan in https://phab.mercurial-scm.org/D1019, it's cleaner if we use defined constants
  instead of `osname == 'nt'` everywhere.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1033

AFFECTED FILES
  mercurial/pycompat.py

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 12, 2017, 6:18 p.m.
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
phabricator - Oct. 12, 2017, 7:19 p.m.
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
phabricator - Oct. 12, 2017, 7:31 p.m.
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
phabricator - Oct. 12, 2017, 7:39 p.m.
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