is_darwin is always False (os.name is never "darwin") #1731
Comments
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this issue
Nov 4, 2023
This fixes compat.is_darwin to use sys.platform instead of os.name so that it is True on macOS systems instead of always being False. The deprecation rationale in compat.py is accordingly adjusted. Together with c01fe76, this largely addresses gitpython-developers#1731.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As discussed in #1679 and #1679 (review), code inside or outside of GitPython that uses
is_winfromgit.compatis prone to bugs, because it is easy to assume wrongly thatis_winwould be true on Cygwin. Instead,is_winis short foros.name == "nt", and writing that out makes clearer what one is testing.However, I have discovered that the potential for confusion in using the
is_<platform>aliases ingit.compatis not limited tois_win. Specifically,is_darwinis alwaysFalse, including on macOS systems.This is because
is_darwinis short foros.name == "darwin", but"darwin"is not currently and seems never to have been one of the possible values ofos.name. In Python 2, there were more values ofos.namethan in Python 3, but in both languagesos.nameis always"posix"on the operating system today called macOS (historically called Mac OS X, with Mac OS 9 and earlier not being POSIX systems).In the infrequent case that one wishes to test for macOS, one can check
sys.platform == "darwin", becausesys.platformis more granular thanos.nameand does take on the value of"darwin"on macOS. GitPython does not currently need to do this, and no uses ofis_darwinappear ingit/ortest/currently. This appears to have been the case since 4545762 (#1295).The buggy definition of
is_darwinseems 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 expressionsys.platform == 'darwin'was used, but it seems to have been inadvertently changed to the always-falseos.name == 'darwin'in that commit. (Theis_<platform>attributes were soon after changed from functions to variables/constants in e61439b, which was also in #519.)Possible fixes
Although GitPython doesn't use
is_darwinanymore, it is a public name ingit.compat(since that module does not define__all__and the nameis_darwinneither 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 onis_darwinhaving the wrong value.Because 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 assumeis_winandis_posixare analogous, examiningsys.platform. In the case ofis_win, which checksos.name = "nt", I believe it is currently equivalent to checkingsys.platform == "win32", though historically this may not always have been the case. In the case ofis_posix, however, it is not equivalent to any straightforward expression withsys.platform, because a few "POSIX" platforms are distinguished with custom values ofsys.platform.Both for this reason and, more so, because of the confusion that turns out to have arisen from the
is_<platform>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 ifDeprecationWarningis emitted, that warning is hidden by default--I thinkis_darwinshould probably be fixed tosys.platform == "darwin"as well, in spite of the disadvantages of doing so.A further benefit of replacing all uses is that static type checkers (and some editors) understand checks against standard-library attributes such as
os.nameand they are able to check code more precisely based on them. This helps with things like flags in thesubprocessmodule 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.I've opened #1732 for this. For the reasons given below, I haven't included any code to emit new
DeprecationWarnings, though I'd be pleased to do so if requested.New warnings?
Although I recommend the
is_<platform>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:__getattr__, but I think that slightly decreases the benefits of static typing. It can also be done by writing a class that inherits fromtypes.ModuleTypeand 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 (though it doesn't discuss static typing implications).DeprecationWarnings on particular module-level attribute accesses will be of use elsewhere in the project. (One possible place is for__version__as discussed in this and that 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.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 ingit.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 ifgit.compatas a whole is deprecated, then emitting aDeprecationWarningwon't require any special attribute handling, as it can just be done with a top-levelwarnings.warncall in the module.The text was updated successfully, but these errors were encountered: