-
-
Notifications
You must be signed in to change notification settings - Fork 908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3.1.38 meant to fix #1656 but there is no supporting gitdb release #1716
Comments
Thanks for bringing this to my attention!
Yes, I'd very much like your help with this, thank you 🙏. |
On further consideration, this might be not be the best time to fix the duplication. This should not stop a patch version of gitdb from being released, so this is kind of a side topic. But my detailed thinking about why it may be better to fix this issue with a new patch version of gitdb without eliminating its metadata duplication follows. Immediate options, and their drawbacksAlthough the repetition is explained in terms of expected style, there is one benefit of it that may be worth preserving: keeping the This could be fixed, without turning smmap or any future dependencies into build dependencies, by any scheme that determines the version without importing the code. This includes the very ad-hoc approach GitPython takes of substituting a version for a placeholder, which I'd be reluctant to introduce to gitdb and would prefer to find a way for GitPython to stop using too (see below), or a reverse ad-hoc approach where the version is hard-coded in packaged files and read by It can also be solved, I think better, with packages that specialize in automatically setting versions from source control tags, such as Slightly longer-term ideaUnlike the situation in #1713--where a downstream build recipe was broken in a specific way that GitPython intended to guarantee would not break--I actually think breaking downstream builds for this sort of thing is totally defensible. However, as a separate goal from de-duplicating version information, I think it would be best for GitPython, gitdb, and smmap to replace all their (Note that if the GitPython repository becomes a monorepo as discussed in gitpython-developers/smmap#53 (comment), the three projects would still be defined separately. So this would go along with or perhaps even support such a change, rather than clashing with or being an alternative to it.) For this reason, I am reluctant to make any changes to any of these projects' That sort of change could really be done at any time. But the current (temporary) skew, which this issue is about, between the current versions of GitPython and gitdb with respect to Should I still open PRs?You might prefer I still open pull requests. You might:
|
Yes, please! Let's not do any changes to the way the packages are defined unless it makes them declarative.
If I understood correctly, I could create a new patch release of |
Alright, I managed and here is the new On another note, it seems like keeping the version duplicated in |
Arguably GitPython's declared dependency on gitdb should have its minimum version raised to the new version. However, I am not really sure that should be done, and if it is to be done, it might be better to wait for feedback from @DeflateAwning (author of #1659 and gitdb#98) on whether things are working (or at least nothing new is broken) when using
Yes, whether or not anything further has to be changed in GitPython in connection with this, making that change in gitdb now should work fine. |
Everything is good! Main reported issues are fixed. Ideal siuation would be a release on gitdb, bump the submodule version, and release gitpython! |
Furthermore, nothing new is broken, as far as I can tell. Someone could double check, but it's probably fine |
The |
It seems to me that this issue can be considered fixed. |
A However, whether or not that is done, I recommend against having a top-level
I think replacing all the non-stub Disadvantages of
|
Thanks again for this fantastic analysis! I was suggesting deprecation primarily due to my ignorance about the available options, and going with…
…seems like the way to go. I assume that the Sticking with Regarding priority, I see how native windows CI support and declarative setuptools can be independent, but I also see why native windows support will help with testing and validating any future change. As always, this choice lies solely with you :). |
If people who want to run the tests didn't install the test extra, they can still install that extra. This simplifies the instructions accordingly. test-requirements.txt is still mentioned near the beginning in case people want to look at it to see dependencies. But the changed code is the only place where the instructions had still said to do anything with those files. A possible disadvantage of this change is that in the rare case that someone following those instructions to run the tests locally didn't do an editable installation, then installing with the extra shouldn't be done editably either. But this doesn't seem like a likely problem, and the new text has an example command with -e to clarify the kind of command whose effect is augmented here. Because of that subtlety, it is not obvious that this change is really justified for purposes of clarity. However, this also helps prepare for a time when test-requirements.txt won't exist anymore, as may happen when the project definition is made fully declarative (see discussion in comments in gitpython-developers#1716).
This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. As noted in c66257e, linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing with gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), is not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It is an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already does not achieve its goal since gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. This commit *avoids* doing any of the following, even though it may be useful to do them later: - This does not give specific instructions in the readme for installing and running ruff (and c66257e before this also omits that). This can be added later and the best way to document it may depend on some other upcoming decisions (see below). - This does not add ruff to the test extra or as any other kind of extra or optional dependency. Although the test extra currently contains some packages not used for running unit tests, such as pre-commit and mypy, adding Ruff will cause installation to take a long time and/or or fail on some platforms like Cygwin where Ruff has to be built from (Rust) source code. This can be solved properly by reorganizing the extras, but that is likely to wait until they are expressed completely inside pyproject.toml rather than one per requirements file (see discussion in comments in gitpython-developers#1716 for general information about possible forthcoming changes in how the project is defined). - This does not update tox.ini to run ruff directly, which could be done by splitting the current lint tox environment into two or three environments for Python linting, Python autoformatting, and the miscellaneous other tasks performed by pre-commit hooks, only the latter of which would use the pre-commit command. Whether and how this should be done may depend on other forthcoming changes. - This does not remove or update the Makefile "lint" target. That target should perhaps not have been added, and was always meant to be improved/replaced, since everything else in the top-level Makefile is for building and publishing releases. See 5d15063 (gitpython-developers#1693). This is likewise not done now since it may depend on as-yet unmerged changes and tooling decisions not yet made. It should be feasible to do together when further updating tox.ini. - This does not update tox.ini, Makefile, or the lint.yml GitHub Actions workflow to omit the manual hook-stage, which will be unused as of gitpython-developers#1865. This would complicate integration of changes, and once it is known that it won't be needed, it can removed. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.
If people who want to run the tests didn't install the test extra, they can still install that extra. This simplifies the instructions accordingly. test-requirements.txt is still mentioned near the beginning in case people want to look at it to see dependencies. But the changed code is the only place where the instructions had still said to do anything with those files. A possible disadvantage of this change is that in the rare case that someone following those instructions to run the tests locally didn't do an editable installation, then installing with the extra shouldn't be done editably either. But this doesn't seem like a likely problem, and the new text has an example command with -e to clarify the kind of command whose effect is augmented here. Because of that subtlety, it is not obvious that this change is really justified for purposes of clarity. However, this also helps prepare for a time when test-requirements.txt won't exist anymore, as may happen when the project definition is made fully declarative (see discussion in comments in gitpython-developers#1716).
Although it seems likely that the requirements-dev.txt file will be removed when the project definition is made declarative (discussed in gitpython-developers#1716 comments), if not before, for now it exists and might be in use, so this updates it with tools that are currently used but not listed in any extras or other requirements files: - ruff: This has replaced flake8 and its plugins (gitpython-developers#1862) as well as black (gitpython-developers#1865). Currently there is no separate extra for tooling that is not part of unit testing, with some such tools listed in test-requirements.txt. The `ruff` package belongs here rather than there for now because it should not be installed just to run unit tests, since Ruff has to be built from source on some rarer platforms like Cygwin, which would take a long time and/or fail. - shellcheck: The PyPI package for this is a convenience for installing it in projects that are already using pip (shellcheck is neither written in Python nor a tool to scan Python code). It installs pre-built binaries, which are not available for all platforms. These packages remain listed: - pytest-icdiff: This seems not to have been promoted to be in the test-requirements.txt file and the `test` extra because it does not work as well for diffs from tests that the pytest runner runs but that are written for the unittest framework. - pytest-profiling [commented out]: I am not sure what the status of this is, perhaps it has just not been experimented with enough to know if it would be useful for profiling in GitPython. This requirements-dev.txt file has a few limitations that suggest it should be removed altogether sometime soon: - It is not updated regularly. - It is not always clear why something is there. Originally I believe it was for tools where the desire to use the tool was established but the tool did not yet work or worked but performed checks for which code had to be fixed. That purpose has drifted. - It uses a different naming convention from the test-requirements.txt file in active use. - It cannot be readily used to create an extra in the current project definition in setup.py because the simple parsing done there will not recognize the `-r` lines and will not skip the comments, and neither enhancement should be done in setup.py since that would move things farther away from a declarative project definition. - It will naturally go away when the project definition is made declarative, since it will then be feasible to define as many extras as desired without proliferating separate requirements files (while still allowing their contents to be statically available to tools). Since it may go away soon and is not regularly updated, I have kept the explanations for why particular packages are there out of it. But as long as it exists it may as well list the tools that really are being used yet are not explicitly listed as dependencies.
To support #1659 here in GitPython, gitdb received #98, and the GitPython repository's submodule reference to gitdb was updated in #1704 (and #1705). Thus #1656 might be1 fixed when developing against the cloned GitPython repository with its gitdb submodule.
With the intention of fixing #1656 in the far more common case of using GitPython via PyPI packages, GitPython 3.1.38 was released. But no new release of gitdb was ever made. Since the fix depends on changes to gitdb, it seems unlikely that 3.1.38 really fixes #1656.
I think it might be possible to make further refinements to
__all__
and surrounding code, slightly enhancing #1659. However, since 3.1.38 was already released, it seems to me that such things can wait, and that it's reasonable to make a gitdb release (bumping the version in itssetup.py
andgitdb/__init__.py
, or changing the current approach and bumping it in just one place), and to include a dependency bump requiring it the next GitPython release (which could be the same release that ships #1715, if the timing works out).I'd be pleased to open pull requests to help with this issue, either following this plan or another one, if that would be helpful.
1 I had originally said it was likely fixed when developing against the cloned GitPython. However, that is probably not the case. First, most type checkers probably wouldn't understand how that version is added to
sys.path
, and it is probably good design of them not to (since they are static checkers). Second, as documented in #1717, it turns out this is never actually happening.The text was updated successfully, but these errors were encountered: