23 convert to pygit2 - #24
Conversation
There was a problem hiding this comment.
Hey Umut, thanks for the PR! The replacement of subprocess works for me and looks great already! I have found a few more places where we can do some work, e.g. use os.path.join and some spots where the error handling could be improved. I also had some questions, would be great if you can comment my review here :)
| subprocess.run(['git', 'add', '.']) | ||
| subprocess.run(['git', 'commit', '-m', 'new gold annotations']) | ||
| subprocess.run(['git', 'push', 'origin', 'HEAD:master']) | ||
| gold_ac.push_annotations(commit_message='new gold annotations') |
There was a problem hiding this comment.
We could add a more specific message here for the commit. This way, users who inspect their repository could have a better overview. My suggestion would be:
gold_ac.push_annotations(commit_message=f'Added {copied_annotations} gold annotations into annotation collection '{gold_ac_name}'.)
| # os.chdir(f'{project.projects_directory}{project.uuid}/collections/{gold_uuid}') | ||
| # subprocess.run(['git', 'add', '.']) | ||
| # subprocess.run(['git', 'commit', '-m', 'new gold annotations']) | ||
| # subprocess.run(['git', 'push', 'origin', 'HEAD:master']) |
There was a problem hiding this comment.
I was wondering if we can remove the lines with the calls to subprocess.
| new_annotation_uuid = uuid_override | ||
|
|
||
| tag_relative_path = tag.path[tag.path.index('/')+1:tag.path.rindex('/')] | ||
| tag_relative_path = os.path.relpath(tag.path, project_path).replace('\\', '/') |
There was a problem hiding this comment.
Since this is a workaround to be able to work with both Linux and Windows filepaths, a short comment on why we replace the slashes would be great.
| new_annotation_uuid = uuid_override | ||
|
|
||
| tag_relative_path = tag.path[tag.path.index('/')+1:tag.path.rindex('/')] | ||
| tag_relative_path = os.path.relpath(tag.path, project_path).replace('\\', '/') |
There was a problem hiding this comment.
What would happen if the tagset or tag doesn't exist? We could add a short error message to the find_tagset_by_name and find_tag_by_name methods and let the user know which tagsets/tags exist in the respective project/tagset.
| if repo.index.conflicts is not None: | ||
| for conflict in repo.index.conflicts: | ||
| print('Conflicts found in:', conflict[0].path) | ||
| raise AssertionError('Conflicts have been found, pull aborted. Please resolve conflicts.') |
There was a problem hiding this comment.
Why are we using the AssertionError error class here? Could we use a general error as well?
There was a problem hiding this comment.
We could use os.path.join here as well.
There was a problem hiding this comment.
Could we use os.path.join here as well to construct the path?
| self.path: str = os.path.join(project_uuid, 'tagsets', tagset_uuid) | ||
|
|
||
| try: | ||
| with open(self.path + '/header.json', 'r', encoding='utf-8', newline='') as header_input: |
There was a problem hiding this comment.
We could use os.path.join here as well.
There was a problem hiding this comment.
I think this file should be removed (or moved with the test_pygit2.ipynb notebook).
…iled pushes to remote
…sage for automated merges
2 participants
os.chdir()calls have been removed and file path operations handled viaos.path.joincalls with absolute paths.Two changes have been made to test files:
../demo/projectschanged intodemo/projects