-
Notifications
You must be signed in to change notification settings - Fork 16
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
tests WIP #26
tests WIP #26
Conversation
… feature/tests
@mike0sv, do we need something else right now in terms of tests? |
gto/api.py
Outdated
from gto.registry import GitRegistry | ||
|
||
|
||
def show(repo: str, dataframe: bool = False): |
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 suggest to use consistent naming for different types. E.g for variable repo
I expect Repo
type. For strings it could be smth like repo_path
. But in this case it could be Union
for easy re-use of existing Repo
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.
btw, maybe we can make this a method and api method will only create registry if needed and call this method?
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.
same goes for other methods
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.
But in this case it could be Union for easy re-use of existing Repo
Done
btw, maybe we can make this a method and api method will only create registry if needed and call this method?
Not sure it's better to have it in registry, the logic is pretty much about some data formatting. Better to decouple IMO.
close #27
close #3
close #9