Skip to content
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

Submodule.remove module parameter doc is unclear or incomplete #1712

Closed
EliahKagan opened this issue Oct 17, 2023 · 2 comments · Fixed by #1714
Closed

Submodule.remove module parameter doc is unclear or incomplete #1712

EliahKagan opened this issue Oct 17, 2023 · 2 comments · Fixed by #1714

Comments

@EliahKagan
Copy link
Contributor

EliahKagan commented Oct 17, 2023

The docstring for Submodule.remove documents the module parameter this way:

:param module: If True, the module checkout we point to will be deleted
as well. If the module is currently on a commit which is not part
of any branch in the remote, if the currently checked out branch
working tree, or untracked files,
is ahead of its tracking branch, if you have modifications in the
In case the removal of the repository fails for these reasons, the
submodule status will not have been altered.
If this submodule has child - modules on its own, these will be deleted
prior to touching the own module.

A sentence trails off in "if you have modifications in the" and it appears material is missing. This should be revised, but it is is unclear exactly what the intended meaning is, so I don't know for sure how to fix it.

The current phrasing appears to be the effect of an unsuccessful or incomplete attempt, in 8e0e315, to fix a similar misphrasing. Compared to the original wording in 3d061a1, the change was:

         :param module: If True, the module we point to will be deleted 
             as well. If the module is currently on a commit which is not part 
             of any branch in the remote, if the currently checked out branch 
-            is ahead of its tracking branch,  if you have modifications in the
             working tree, or untracked files,
+            is ahead of its tracking branch,  if you have modifications in the
             In case the removal of the repository fails for these reasons, the 
             submodule status will not have been altered.
             If this submodule has child-modules on its own, these will be deleted

So the original wording also trailed off in mid-sentence, but differently, with "working tree, or untracked files,".

I can guess the intended meaning:

If True, the module checkout we point to will be deleted as well. If the module is currently on a commit which is not part of any branch in the remote, if the currently checked out branch is ahead of its tracking branch, or if there are modifications to the working tree or untracked files, then the removal will fail.

If that revision is correct, it could be clarified by a small amount of further rewording:

If True, the checked out module we point to will be deleted as well. If that module is currently on a commit outside any branch in the remote, or if it is ahead of its tracking branch, or if there are modified or untracked files in its working tree, then the removal will fail.

However, I am not sure enough that this is the intended meaning to have put either of those changes in a PR at this time.

@Byron
Copy link
Member

Byron commented Oct 18, 2023

Thanks for bringing this up.

I am also not sure and I suppose the tests or the code itself could reveal more of the story. In the meantime, I think it's OK to go with the second version of the proposed correction as seemingly correct seems better than incomplete and nonsensical.

The submodule implementation is another wart of the project, unfortunately, as it's a half-baked attempt to improve git submodules which has many, many issues.

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Oct 18, 2023
This rewords and adds some missing information to the docstring of
Submodule.remove, for the "method" parameter, discussed in gitpython-developers#1712.
It uses the second suggestion presented in that issue, formatted
in the style of the surrounding docstring (an 88-column wrap).

It also does some other rewording in that docstring, for clarity,
at the end of that same section (on the "method" parameter), and a
small punctuation fix in the section about the "force" parameter.
@EliahKagan
Copy link
Contributor Author

In the meantime, I think it's OK to go with the second version

Sounds good. I've done this in #1714.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants