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

Deprecate upload and register commands #1410

Merged
merged 2 commits into from Jul 9, 2018

Conversation

Projects
None yet
3 participants
@di
Member

di commented Jul 8, 2018

Summary of changes

As discussed in #1381, this PR deprecates the upload and register commands by warning the user not to use these commands.

Pull Request Checklist

  • News fragment added in changelog.d. See documentation for details
# Make sure that we are using valid current name/version info
self.run_command('egg_info')
orig.register.run(self)
self._deprecation_notice()

This comment has been minimized.

@pganssle

pganssle Jul 8, 2018

Member

I'm not sure I understand why the try/catch here.

Can we do this?

self._deprecation_notice()
self.run_command('egg_info')
orig.register.run(self)

Is it because run_command and register.run generate a bunch of text, and you want the deprecation warning to be printed after this text?

If so, maybe this would be better:

try:
    self.run_command('egg_info')
    orig.register.run(self)
except Exception:
    raise
finally:
    self._deprecation_notice()

This comment has been minimized.

@di

di Jul 8, 2018

Member

Yeah, exactly. I'll switch this to using finally.

@pganssle

This comment has been minimized.

Member

pganssle commented Jul 8, 2018

I'm +1 on this initiative, but I do think it would be nice in the documentation for register and upload if we actually provide the specific 1-to-1 equivalents in addition to just linking to the twine documentation. I often find that it's way easier to move over when I don't have to learn a whole new tool just to figure out how to do the equivalent of what I used to do with the old tool.

Also, ideally this would have tests, though I'm not super familiar with the announce mechanism used here and how easy it would be to test that. Maybe the best thing to do is to use a mock of some sort to make sure that announce is called with some regex matching something like .*[Rr]egister.*deprecated.*twine.* (or matching all three patterns [Rr]egister, [Dd]eprecated, twine).

@di di force-pushed the di:deprecate-upload-and-register branch from 9e2d19b to 44c420c Jul 8, 2018

@di

This comment has been minimized.

Member

di commented Jul 9, 2018

I'm +1 on this initiative, but I do think it would be nice in the documentation for register and upload if we actually provide the specific 1-to-1 equivalents in addition to just linking to the twine documentation.

I kinda disagree with this, I don't think it should be setuptools's job to tell folks how to use twine.

di added some commits Jul 8, 2018

@di di force-pushed the di:deprecate-upload-and-register branch from ba9caeb to 7edce73 Jul 9, 2018

@pganssle

This comment has been minimized.

Member

pganssle commented Jul 9, 2018

I don't think it needs to block this particular PR if you don't want to do it, but the problem is that it has to be someone's job, so either twine should have a "migrating from setuptools" section or setuptools should have a "migrating to twine" section, and whichever one has it, the other one should link to it.

Yes we can just say, "Go ahead and read the twine documentation", but as a practical matter "If you are trying to do X with setuptools, do Y with twine instead!" will get people switching over much faster than just "Use twine for this instead!"

@pganssle

This comment has been minimized.

Member

pganssle commented Jul 9, 2018

I think we can link to this in the setuptools documentation for upload. There doesn't seem to be an anchor target for twine register (which I guess is unnecessary now anyway).

@pganssle pganssle closed this Jul 9, 2018

@pganssle pganssle reopened this Jul 9, 2018

@pganssle

This comment has been minimized.

Member

pganssle commented Jul 9, 2018

I think the netlify deployment failure was an intermittent failure. I don't seem to have permissions to restart it, though. Close-and-reopen didn't seem to work. @ewdurbin Do you own the netlify account associated here? Do you know if you can give me and the other setuptools maintainers permissions on it?

@ewdurbin

This comment has been minimized.

Member

ewdurbin commented Jul 9, 2018

@pganssle @jaraco was invited as an administrator on the project during PyCon sprints. Can you private message me an email address you'd prefer for me to add yourself as well?

@di

This comment has been minimized.

Member

di commented Jul 9, 2018

I think it would be reasonable for twine to have a "migrating from setuptools" guide, let's see what the maintainers there think.

@pganssle

Awesome, LGTM. We can do a later PR to add links to any potential "migrating from setuptools" section in twine.

@pganssle pganssle merged commit 54ce659 into pypa:master Jul 9, 2018

5 checks passed

codecov/patch 100% of diff hit (target 80.99%)
Details
codecov/project 81.1% (+0.1%) compared to 244ff32
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
New in 20.1: Added keyring support.
New in 40.0: Deprecated the upload command.

This comment has been minimized.

@di

di Jul 9, 2018

Member

@pganssle Since there was a 40.0 release last night, we'll need to tweak this line before releasing. I took a guess at what version this would land in, but seeing as it's a minor change, this should probably be 40.1.

@di di deleted the di:deprecate-upload-and-register branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment