René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"is_darwin is always False (os.name is never \"darwin\")","articleBody":"As discussed in #1679 and https://github.com/gitpython-developers/GitPython/pull/1679#pullrequestreview-1644389896, code inside or outside of GitPython that uses `is_win` from `git.compat` is prone to bugs, because it is easy to assume wrongly that `is_win` would be true on Cygwin. Instead, `is_win` is short for `os.name == \"nt\"`, and writing that out makes clearer what one is testing.\r\n\r\nHowever, I have discovered that the potential for confusion in using the `is_\u003cplatform\u003e` aliases in `git.compat` is not limited to `is_win`. Specifically, **`is_darwin` is always `False`, including on macOS systems**.\r\n\r\nThis is because `is_darwin` is short for `os.name == \"darwin\"`, but `\"darwin\"` [is not currently](https://docs.python.org/3/library/os.html#os.name) and [seems never to have been](https://docs.python.org/2.7/library/os.html#os.name) one of the possible values of `os.name`. In Python 2, there were more values of `os.name` than in Python 3, but in both languages `os.name` is always `\"posix\"` on the operating system today called macOS (historically called Mac OS X, with Mac OS 9 and earlier not being POSIX systems).\r\n\r\nIn the infrequent case that one wishes to test for macOS, one can check `sys.platform == \"darwin\"`, because `sys.platform` is more granular than `os.name` and does take on the value of `\"darwin\"` on macOS. GitPython does not currently need to do this, and no uses of `is_darwin` appear in `git/` or `test/` currently. This appears to have been the case since 4545762 (#1295).\r\n\r\nThe buggy definition of `is_darwin` seems to have had limited impact in the project, having been introduced in f495e94 (part of #519) and only used in a couple of places, then none since #1295. Before #519, the correct expression `sys.platform == 'darwin'` was used, but it seems to have been inadvertently changed to the always-false `os.name == 'darwin'` [in that commit](https://github.com/gitpython-developers/GitPython/commit/f495e94028bfddc264727ffc464cd694ddd05ab8#diff-2f2b548c14a71e5bbf15502f6d7fd98a50842119152c451ae2ec5e1cc42f02d2). (The `is_\u003cplatform\u003e` attributes were soon after changed from functions to variables/constants in e61439b, which was also in #519.)\r\n\r\n### Possible fixes\r\n\r\nAlthough GitPython doesn't use `is_darwin` anymore, it is a public name in `git.compat` (since that module does not define `__all__` and the name `is_darwin` neither starts with a `_` nor has been documented as nonpublic). So it may be in other projects that use GitPython. If so, those other projects suffer either from this bug, or if not then from another bug of their own where they depend on `is_darwin` having the wrong value.\r\n\r\nBecause removing it would be a breaking change, it should probably be kept, but fixed to use the expression `sys.platform == \"darwin\"`. However, this does have the disadvantage that a developer who checks its implementation may assume `is_win` and `is_posix` are analogous, examining `sys.platform`. In the case of `is_win`, which checks `os.name = \"nt\"`, I believe it is currently equivalent to checking `sys.platform == \"win32\"`, though historically this may not always have been the case. In the case of `is_posix`, however, it is not equivalent to any straightforward expression with `sys.platform`, because a few \"POSIX\" platforms are distinguished with custom values of `sys.platform`.\r\n\r\nBoth for this reason and, more so, because of the confusion that turns out to have arisen from the `is_\u003cplatform\u003e` aliases, I recommend all of them be replaced, and also that they be documented as deprecated. Because deprecation is often not readily discovered when one is not looking for it--even if `DeprecationWarning` is emitted, that warning is hidden by default--I think `is_darwin` should probably be fixed to `sys.platform == \"darwin\"` as well, in spite of the disadvantages of doing so.\r\n\r\nA further benefit of replacing all uses is that static type checkers (and some editors) understand checks against standard-library attributes such as `os.name` and they are able to check code more precisely based on them. This helps with things like flags in the `subprocess` module that differ across platforms (though attribute accesses based on them have to be written across separate expressions, rather than for example in a ternary expression, to get the full benefit of these distinctions, at least with current type checkers). As far as I know, there's no fundamental reason this shouldn't also work with module attributes assigned that kind of expression (typing it automatically as a literal value), but that does not seem to happen.\r\n\r\nI've opened #1732 for this. For the reasons given below, I haven't included any code to emit new `DeprecationWarning`s, though I'd be pleased to do so if requested.\r\n\r\n### New warnings?\r\n\r\nAlthough I recommend the `is_\u003cplatform\u003e` attributes be deprecated, I am unsure if it is worthwhile to have accesses to those attributes raise any warnings at this time. There are a few considerations:\r\n\r\n- This can be done by defining a module-level `__getattr__`, but I think that slightly decreases the benefits of static typing. It can also be done by writing a class that inherits from `types.ModuleType` and defines properties, and assigning that class to the `__class__` attribute of the module object. This may be preferable, *if* it turns out to play better with static typing. See [Customizing module attribute access](https://docs.python.org/3/reference/datamodel.html#customizing-module-attribute-access) (though it doesn't discuss static typing implications).\r\n- It seems very likely that `DeprecationWarning`s on particular module-level attribute accesses will be of use elsewhere in the project. (One possible place is for `__version__` as discussed in [this](https://github.com/gitpython-developers/GitPython/issues/1716#issuecomment-1773728098) and [that](https://github.com/gitpython-developers/GitPython/issues/1716#issuecomment-1773746757) comment, but I think there may be a number of others, too.) But I'd be more comfortable introducing either of these things once static typing is in better shape, so their effects on that can be better judged.\r\n- There is also a more specific and, in my view, important reason to be reluctant to add this functionality for those attributes right now: they are in `git.compat`, which could perhaps be *entirely* deprecated sometime soon. That module's original purpose was to help in supporting both Python 2 and Python 3. Since GitPython currently only supports Python 3, it might be possible to eliminate use of *all* of the facilities in `git.compat`, though whether or not that *should* be done is another question, since some uses of it might be considered to improve clarity for reasons not tied to the distinction between Python 2 and Python 3. But if `git.compat` as a whole is deprecated, then emitting a `DeprecationWarning` won't require any special attribute handling, as it can just be done with a top-level `warnings.warn` call in the module.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-11-04T21:20:33.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":0},"url":"https://github.com/1731/GitPython/issues/1731"}