Skip to content
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

core: add tag_create method #120

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Jan 18, 2021

This adds a new tag_create method, using dolt tag CLI verb for
tag creation.

This adds a new `tag_create` method, using `dolt tag` CLI verb for
tag creation.
@oscarbatori
Copy link
Contributor

@lucab thanks for this contribution, would you mind naming the method tag instead of tag_create, as we are trying to stick closely to the command line interface in terms of naming convention?

@lucab
Copy link
Contributor Author

lucab commented Jan 19, 2021

@oscarbatori the problem with dolt tag is that conceptually it can do tag_list + tag_create + tag_delete in a single verb.
I can rename this creation method to simply tag, but then I don't see how to add listing and deleting capabilities in the future on the Python side. Thoughts on how to proceed on that?

@oscarbatori
Copy link
Contributor

@max-hoffman may have some views here, and he is going to be taking over Doltpy as dev lead. My general take is that we made a decision to replicate the CLI exactly in order to make it easy to make the transition. I think you correctly point out that this leads to some Python anti-patterns, though since they basically just pass commands into Popen calls I think the right call is the easy UX.

Our next release will ensure doltpy.cli is the only package with this kind of ugliness and we will expose a higher level API elsewhere that is more natural in Python.

So, I think for now we want to have this sort of ugly parameter pattern and then possibly expose something more Pythonic in doltpy.manage or whatever.

@max-hoffman
Copy link
Collaborator

Thanks @oscarbatori , and you bring up good points @lucab.

Generally we want to provide an interface that will be backwards compatible indefinitely. Pinning to MySQL and Git semantics is a good starting point. Identifying sections of dolt whose interfaces are likely to change with the progression of data science tools is also helpful.

For structuring the code, I am looking into how other libraries organize these sorts of interfaces:

If you have other comments, suggestions or ideas we'd be interested to hear. I am still getting up to speed, and refactoring the interface will be an on-going process.

@lucab
Copy link
Contributor Author

lucab commented Jan 26, 2021

The git API looks quite nice IMHO: a Tag class with a create class-method.
However that would depart quite a bit from current API approach and will invert current relation (Tag::create(repo, name) vs repo.tag(name)).

I'm not in a hurry for this, so I'll be happy to let you settle on future API directions and the rework this the way you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants