Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Remove optional from two member variables#1550
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Remove optional from two member variables#1550
Uh oh!
There was an error while loading.Please reload this page.
Conversation
737ec27 to8e8b5d5Compare8e8b5d5 tocc71f02CompareThe changes to |
There was a problem hiding this 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. Ifworking_dir cannot be none, I'd like to see what happens with bare repositories. What kind ofworking_dir are they expected to have?
A bare repo should still have working_dir afaik |
There was a problem hiding this 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 differentworking* 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) | |||
There was a problem hiding this comment.
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 theworking_tree_dir for completeness? It's easy to confuse both which is what I did apparently. Theworking_dir is the CWD of the git repository, either theworking_tree_dir or thegit_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok so the docs inbase.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.
There was a problem hiding this comment.
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
Thanks a lot for bearing with me, I was confused by the difference between And thanks for this great contribution :)! |
Neither
working_dirnorgit_dirappear to leave__init__()set toNone. Let's remove theNoneinitialization, and simply use them as type markers.Fixes#1549