Skip to content

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

@SineaggiSineaggiforce-pushed the remove-optional-from-two-variables branch 5 times, most recently from 737ec27 to 8e8b5d5CompareJanuary 27, 2023 21:41
@SineaggiSineaggiforce-pushed the remove-optional-from-two-variables branch from 8e8b5d5 to cc71f02CompareJanuary 27, 2023 22:20
@Sineaggi
Copy link
ContributorAuthor

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

@ByronByron 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
ContributorAuthor

A bare repo should still have working_dir afaik

Copy link
Member

@ByronByron 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?

deftest_with_bare_rw_repo(self, bare_rw_repo):
assertbare_rw_repo.config_reader("repository").getboolean("core", "bare")
assertosp.isfile(osp.join(bare_rw_repo.git_dir, "HEAD"))
assertosp.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
ContributorAuthor

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

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

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

Copy link
ContributorAuthor

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 :)!

@ByronByron merged commit c84dde2 into gitpython-developers:mainFeb 2, 2023
@ByronByron added this to the v3.1.31 - Bugfixes milestone Feb 2, 2023
@SineaggiSineaggi deleted the remove-optional-from-two-variables branch February 2, 2023 21:28
Sign up for freeto 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

@Sineaggi@Byron