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

Add proposal for real Twine API #361

Closed
wants to merge 1 commit into from
Closed

Add proposal for real Twine API #361

wants to merge 1 commit into from

Conversation

sigmavirus24
Copy link
Member

This is a written proposal for the design of a real Twine API for other
people as well as the plan for work.

cc @reinout @htgoebel

Refs #194

This is a written proposal for the design of a real Twine API for other
people as well as the plan for work.
@codecov

This comment has been minimized.

sigmavirus24 added a commit that referenced this pull request May 17, 2018
This allows us to design the APIs so that they only take keyword
arguments to make it easier and more explicit for users.

Refs #361
sigmavirus24 added a commit that referenced this pull request May 17, 2018
This adds the Settings object described in our API specification so
folks can get a feeling for how it will work and what it will be
responsible for.

Refs #361
sigmavirus24 added a commit that referenced this pull request May 17, 2018
Let's start internally using our new API and dogfood it. This will allow
us to start understanding the impact of this new API.

Refs #361
@mauritsvanrees
Copy link
Contributor

[I used a wrong command, prematurely submitting a previous comment which I have meanwhile removed.]

Thanks for working on this!

Let me see how zest.releaser is currently using twine. See release.py (version 6.15.0 to give a stable link). Simplified a bit, it boils down to this:

from twine.repository import Repository
from twine.package import PackageFile

class Releaser(...):

    def _upload_distributions(self, package):
        ... create sdist and bdist_wheel
        files_in_dist = sorted list of files in dist directory
        servers = ... get servers/repositories by parsing ~/.pypirc ourselves.
        for server in servers:
            # possibly call 'register' but by default only 'upload'
            for filename in files_in_dist:
                self._retry_twine('upload', server, filename)

    def _retry_twine(self, twine_command, server, filename):
        # repository config is read from pypirc.
        repository = Repository(repository_url=X, username=Y, password=Z)
        package_file = PackageFile.from_filename(filename, comment=None)
        already_uploaded = repository.package_is_uploaded(package_file)
        if already_uploaded:
            return
        response = repository.upload(package_file)
        if response is not None and response.status_code == codes.OK:
            return
        # Something went wrong.  Close repository.
        repository.close()

I am too sleepy to really see if the proposed api fits this, but it seems it should be possibly to let zest.releaser use the proposed api.

@htgoebel
Copy link

For zest.release, a much simpler API would suffice (see #194 (comment) and zestsoftware/zest.releaser#238 (comment)):

twine.cli.dispatch(twine_command, '-r', server, *files_in_dist)

This is already implemented at zestsoftware/zest.releaser#284.

@sigmavirus24
Copy link
Member Author

@htgoebel that may be "simple" but it also provides very little flexibility. Further, twine.cli will likely become internal/private as a result of the work to provide an API that has been designed for external (and internal) use.

@sigmavirus24
Copy link
Member Author

@htgoebel Perhaps we can work out more of a porcelain API though. What about something that looks like:

from twine import api as twine


uploader = twine.UploadBuilder().read_config_from(
    '~/.pypirc'
).enable_signing_with(
    identity='...',
    signing_binary='gpg2',
).find_distributions_in(
    path='./dist',
).authenticate_with(
    username='...',
    password='...',
)

result = uploader.complete()

@brainwane brainwane removed their assignment Jul 13, 2018

from twine import api as twine

settings = twine.SettingsBuilder().authenticate_with(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder pattern doesn't look typical for Python apps. I'd say that we're more used to having context managers and we expect object initializer (__init__ method) to exist with fully initialized and validated instance.

@htgoebel
Copy link

@sigmavirus24 For what I need such a porcelain API is much too complicated. It could require the other program to duplicate a lot of code for reading the config, which twine already does.

@reinout
Copy link
Contributor

reinout commented Jan 23, 2019

I agree with @htgoebel that the porcelain would be much too complicated for our use case.

@sigmavirus24, you say it may be "simple" but it also provides very little flexibility. Yes, exactly, that's what zest.releaser wants. We don't need flexibility. Anything that's flexible needs to be configured. The porcelain suggest we should pass along the allowed signing methods: we don't want that as we'd have to configure it.

Where we originally came from, 12 years ago or so, was code that called python setup.py upload on the command line. Setuptools would look in its own .pypirc file and grab user/pass from there if it wanted.

I mean, calling twine upload on the command line works. You don't have to call twine upload --yes-use-pypirc --allow-signing-with-gpg --yes-also-allow-keychain --yes-xyz. The equivalent of twine upload is plenty enough for us.

So please don't make the perfectly-fine "cli" api hidden or private or so.

@sigmavirus24
Copy link
Member Author

So please don't make the perfectly-fine "cli" api hidden or private or so.

It's always been documented as not for your use and it's already caused conflict due to your using it regardless as we change that.

To be clear, I think the example I provided was definitely more than most would want and people would likely only need:

uploader = twine.UploadBuilder().read_config_from(
    '~/.pypirc'
).find_distributions_in(
    path='./dist',
)

result = uploader.complete()

@htgoebel
Copy link

@sigmavirus24 This still requires the embedding tool to duplicate twine's code and internals.

Please understand that we are perfectly happy with a high-level interface mimicking the command line interface. zest.release simply wants to avoid spawning a process.

For me it looks like you want some sophisticated API which to agree on takes loooong (9 moths now without any notable progress) and does not mett user's needs. IMHO this is a waste of your valuable time, since a much simpler API would suffice. I strongly suggest you simply document twine.cli.dispatch() (or some twine.cli.run() and this is done.

@htgoebel
Copy link

I forgot:

To be clear, I think the example I provided was definitely more than most would want and people would likely only need:

Exactly, this is more than most would want. Since most would want to not hazle with config-files, etc.

@reinout
Copy link
Contributor

reinout commented Jan 23, 2019

That example looks less scary already :-) Why would read_config_from('~/.pypirc') be needed, btw? Isn't that the default?

I'd preferably let twine handle the uploading without me getting in the way. We're placing sdists and/or wheels in dist/, so something like this would be handy:

import twine
twine.upload(path='./dist')  # or just .upload() if "dist" is also the default.

Ideally, if someone would need a customization, you'd be able to place a setting in setup.cfg or pyproject.toml in the current directory. Just like pytest and flake8 and zest.releaser look in there for extra settings.

@sigmavirus24
Copy link
Member Author

I strongly suggest you simply document twine.cli.dispatch() (or some twine.cli.run() and this is done.

That will never be a stable API. So that seems like an incredibly horrible thing to do to API end users

@reinout
Copy link
Contributor

reinout commented Jan 24, 2019

So... the only possible API is an elaborate API that doesn't exist yet.
Any simple API is disallowed. You'll even change the current command line method to make sure that the method can never be used as an API.
So we're forced to just call the tool on the command line.

Is calling the tool on the command line allowed? Or are you going to change the command line parameters, too, as "they have never been intended to be used as an API"?

I'm sorely tempted to check whether calling python setup.py upload still works...

@mauritsvanrees
Copy link
Contributor

To be fair, zest.releaser has been using some stable bits from twine for years, twine.repository.Repository and twine.package.PackageFile. As far as I know, those are intended to remain stable. So we have that, which is good. But they required more code, and more understanding of twine internals, than I wanted.

Something like this could work for us I would say, and would enable to replace our current code:

  • twine.api.upload('filepath')
  • twine.api.package_is_registered('package.name')
  • twine.api.register('filepath') preferably too, although not necessary for PyPI.
  • Those could all benefit from an optional keyword argument server to query a different server from the list of index-servers in .pypirc. This could be something for a Settings or Builder object, if you prefer to not have keyword arguments.
  • twine.api.check('filepath') would probably be a good extra, although zest.releaser is currently not using it.
  • The return value in all cases can likely be True for success and False for failure.

A Settings object like you propose could work, but I really hope that this is not needed for cases where the commandline equivalent is just twine upload dist/filename.

@sigmavirus24
Copy link
Member Author

But they required more code, and more understanding of twine internals, than I wanted.

Which is why we're trying to come up with something that requires less internal knowledge.

The key thing in this proposal is that Twine is going to dog-food this API. The way it works now is terrible internally and trying to build up the layers is what would be ideal to me so that we can use it conveniently internally but also allow for a richer ecosystem ala zest releaser, and other tools.

My reasoning for allowing configurability in the API is that I don't think everyone will want to implement the same set of features. Further, in my opinion, it's way too magical when libraries just arbitrarily read files on disk. I understand setuptools has always done this, but it introduces a layer of complexity when users of the library think "I didn't want it to do that."

Also, I believe the Settings work is already merged and being used internally. It also has the ability to register the arguments it requires at a base level so they match twine's and extract the settings from parsed cli args so that the work is contained in Twine.

Those simple functions could absolutely take something like a settings object.

@htgoebel
Copy link

Which is why we're trying to come up with something that requires less internal knowledge.

The least internal knowledge is required by an API which allows passing options just like one the cli, just without the need to find the executable and spawn a process.

Why is this so hard for you to understand? Why can such a simple API not be stable?

I really start becoming angry about you wasting other developer's time while ignoring the uses needs.

@theacodes
Copy link
Member

@htgoebel please keep in mind that participating in this project means following our Code of Conduct. Consider this a warning.

@sigmavirus24
Copy link
Member Author

@htgoebel I'm not trying to waste anyone's time. I'm agreeing with the design @mauritsvanrees has proposed.

If I'm not mistaken you want an API like what Maurits proposed. I haven't had the opportunity to update the document to reflect it. I think the lines of communication have been difficult because I've been hearing an insistence on just using twine.cli.upload which has more downsides than would be even remotely good for a public API. I promise you, I'm not trying to insult you and I'm sorry I've behaved in such a way to make you think that.

@duckinator
Copy link

To provide another viewpoint on this: my project, bork, literally just does from twine.cli import dispatch as twine_cli then calls twine_cli(['upload', '--repository-url', self.repo_url, *files]).

Where self.repo_url is https://pypi.org/simple , https://test.pypi.org/simple , or a compatible endpoint.

So perhaps a variant of @mauritsvanrees' API, like twine.api.upload('file_path', repository_url='repo url') could work? (With repository_url defaulting to None` or something.)

It'd be nice to be able to upload multiple files at once, but I think if I can set the repo URL then I can just call it in a loop and have the same functionality, right? That's not too bad.

@jaraco
Copy link
Member

jaraco commented May 30, 2020

Are there any actions pending on this proposal? Can it be closed pending further action?

@sigmavirus24
Copy link
Member Author

I don't understand your question @jaraco ... As far as I can tell, we need to add the porcelain API proposal that everyone keeps restating slightly differently. (And then implementing the API.)

@jaraco
Copy link
Member

jaraco commented May 30, 2020

It just appears to have stalled. If it's ready to merge, let's merge it. If it requires further action, what action is that? If it's not suitable to merge and there's no plans to address that, then let's close it (until such a time that someone revives the effort).

@htgoebel
Copy link

htgoebel commented May 31, 2020

As already state in #361 (comment):

For our needs such a porcelain API is much too complicated. It could require the other program to duplicate a lot of code for reading the config, which twine already does.
We are perfectly happy with a high-level interface mimicking the command line interface. zest.release simply wants to avoid spawning a process.

@sigmavirus24
Copy link
Member Author

@htgoebel and this is why I stopped working on the proposal. No matter how vehemently I agree with you, you disagree with me for reasons I can't understand.

@jaraco
Copy link
Member

jaraco commented May 31, 2020

One thing that's long confused me - is the point of this PR to add a proposal to the documentation, to be considered alongside other proposals? Or is the pr and the proposal one and the same and if accepted becomes the API doc?

My intention was not to re-open old wounds, but to move things forward. Since I was unsuccessful at that and since this issue has lingered for so long, I believe our best course of action will be to close this proposal as not accepted and use the lessons herein to inform and refine requirements in #194. How does that sound?

@sigmavirus24
Copy link
Member Author

The goal was to use the PR to discuss the proposal and merge with some rough version of consensus. Unfortunately, we haven't been able to agree on the goals of having a documented and implemented API and instead have been hyper-focused on very narrow parts of the discussion.

@jaraco
Copy link
Member

jaraco commented Jun 20, 2020

Without objection, I'm closing this PR. Happy to revive it later or consider a new one.

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.

9 participants