Remote git registry #3
Conversation
…tead of @clone_on_remote_repo
…in, which can be re-used
There was a problem hiding this comment.
How does it look in your opinion? Do you think it's a promising direction?
I guess this is exactly what we have discussed.
(Besides, we could just remove API (but that would require moving some things like show somewhere, and moving with GitRegistry... to cli.py). Feel free to skip that if you didn't plan to do it and I'll do that after your PR as a follow-up. This can also be done in chunk with that optimization I told few times already, so it will be a big change... Just thinking out loud here.)
UPD: TBH, right now it's hard to tell how much this will simplify things, except for general considerations we already had in iterative#296
gto/api.py
Outdated
| if isinstance(path, str) and is_url_of_remote_repo(repo=path): | ||
| try: | ||
| with cloned_git_repo(repo=path) as tmp_dir: | ||
| return init_index_manager(path=repo).add( | ||
| name, | ||
| type=type, | ||
| path=tmp_dir, | ||
| must_exist=must_exist, | ||
| labels=labels, | ||
| description=description, | ||
| update=True, | ||
| ) | ||
| except (NotADirectoryError, PermissionError) as e: | ||
| raise e.__class__( | ||
| "Are you using windows with python < 3.9? " | ||
| "This may be the reason of this error: https://bugs.python.org/issue42796. " | ||
| "Consider upgrading python." | ||
| ) from e | ||
| else: | ||
| return init_index_manager(path=repo).add( |
There was a problem hiding this comment.
Notice how many rather intrusive changes were required. The most annoying part is init_index_manager, which I only started to address.
Yes, I guess here I was unable to properly generalize here, to simplify future changes. Generally, this should be moved to RepoIndexManager I guess.
gto/git_utils.py
Outdated
| class FromRemoteRepoMixin: | ||
| @classmethod | ||
| @abstractmethod | ||
| def _from_repo(cls, repo: Union[str, Repo], config: RegistryConfig = None): | ||
| pass | ||
|
|
||
| @classmethod | ||
| @contextmanager | ||
| def from_repo(cls, repo: Union[str, Repo], config: RegistryConfig = None): | ||
| if isinstance(repo, str) and is_url_of_remote_repo(repo=repo): | ||
| try: | ||
| with cloned_git_repo(repo=repo) as tmp_dir: | ||
| yield cls._from_repo(repo=tmp_dir, config=config) | ||
| except (NotADirectoryError, PermissionError) as e: | ||
| raise e.__class__( | ||
| "Are you using windows with python < 3.9? " | ||
| "This may be the reason of this error: https://bugs.python.org/issue42796. " | ||
| "Consider upgrading python." | ||
| ) from e | ||
| else: | ||
| yield cls._from_repo(repo=repo, config=config) |
There was a problem hiding this comment.
Notice also that the same exact changes were required for GitRegistry and EnrichmentManager. To avoid code duplication I introduced FromRemoteRepoMixin, which is basically the equivalent of @clone_on_remote_repo, just less general.
Yes, I guess this is a good decision - cause this is needed in different classes.
…ing Repo Co-authored-by: Alexander Guschin <1aguschin@gmail.com>
Some fixes + further work
Other option to handle `is_url_of_remote_repo`
delete set_push_on_remote_repo
No description provided.