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

Remove optional from two member variables #1550

Merged

Conversation

Sineaggi
Copy link
Contributor

Neither working_dir nor git_dir appear to leave __init__() set to None. Let's remove the None initialization, and simply use them as type markers.

Fixes #1549

@Sineaggi Sineaggi force-pushed the remove-optional-from-two-variables branch 5 times, most recently from 737ec27 to 8e8b5d5 Compare January 27, 2023 21:41
@Sineaggi Sineaggi force-pushed the remove-optional-from-two-variables branch from 8e8b5d5 to cc71f02 Compare January 27, 2023 22:20
@Sineaggi
Copy link
Contributor Author

The changes to _get_config_path and _config_reader were necessary as these functions are called during __init__, sometimes before git_path is initialized.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a test-driven approach. If working_dir cannot be none, I'd like to see what happens with bare repositories. What kind of working_dir are they expected to have?

@Sineaggi
Copy link
Contributor Author

Sineaggi commented Feb 1, 2023

A bare repo should still have working_dir afaik

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up.

Can you try to add clearer assertions to the different repo types, bare and non-bare, that make the relationship of these different working* directories (even more) clear?

@@ -98,6 +98,7 @@ def test_object_resolution(self):
def test_with_bare_rw_repo(self, bare_rw_repo):
assert bare_rw_repo.config_reader("repository").getboolean("core", "bare")
assert osp.isfile(osp.join(bare_rw_repo.git_dir, "HEAD"))
assert osp.isdir(bare_rw_repo.working_dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Could you also add an assertion of the working_tree_dir for completeness? It's easy to confuse both which is what I did apparently. The working_dir is the CWD of the git repository, either the working_tree_dir or the git_dir, which I think should be an assertion for the bare and non-bare cases.

If that assertion holds, of course, I was going by the docs.

Screenshot 2023-02-02 at 07 15 07

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so the docs in base.py say that working_tree_dir can return none

    @property
    def working_tree_dir(self) -> Optional[PathLike]:
        """:return: The working tree directory of our git repository. If this is a bare repository, None is returned."""
        return self._working_tree_dir

I've added a few more assertions, please check if I understood you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also updated the python docs of the class to mention returning None

@Byron
Copy link
Member

Byron commented Feb 2, 2023

Thanks a lot for bearing with me, I was confused by the difference between working_dir and working_tree_dir, especially since there is a work_dir in gitoxide (the equivalent of working_tree_dir).

And thanks for this great contribution :)!

@Byron Byron merged commit c84dde2 into gitpython-developers:main Feb 2, 2023
@Byron Byron added this to the v3.1.31 - Bugfixes milestone Feb 2, 2023
@Sineaggi Sineaggi deleted the remove-optional-from-two-variables branch February 2, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

working_dir has Optional type when it can't be None
2 participants