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 requires #1541

Merged
merged 3 commits into from Oct 29, 2018

Conversation

3 participants
@smenon8
Contributor

smenon8 commented Oct 27, 2018

Summary of changes

Will throw a DeprecatedWarning whenever requires parameter is used.

Closes #1374

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
@pganssle

This comment has been minimized.

Member

pganssle commented Oct 27, 2018

@smenon8 We actually want to deprecate this parameter, not remove it, that means that when someone specifies requires they should see a deprecation warning. I think this will cause requires to be silently ignored, or possibly throw an error?

@smenon8

This comment has been minimized.

Contributor

smenon8 commented Oct 27, 2018

@pganssle I see, sorry, I misunderstood.

@pganssle pganssle moved this from Approved PRs to Submitted PRs in PyPA Sprint Weekend at Bloomberg (2018) Oct 27, 2018

@@ -399,6 +401,20 @@ def parse(self):
section_parser_method(section_options)
def deprecated_config_handler(self, msg, warning_class, func):

This comment has been minimized.

@pganssle

pganssle Oct 27, 2018

Member

This is a bit of a nitpick, but I'd prefer if func came first in this case.

This comment has been minimized.

@pganssle

pganssle Oct 27, 2018

Member

Also can you name this _deprecated_config_handler? We do not want this to be considered part of the public interface.

This comment has been minimized.

@smenon8

smenon8 Oct 27, 2018

Contributor

Sure, let me make that change.

'requires = some, requirement\n'
)
with pytest.raises(DeprecationWarning):

This comment has been minimized.

@pganssle

pganssle Oct 27, 2018

Member

I think this needs to be pytest.warns.

'requires = some, requirement\n'
)
with pytest.raises(DeprecationWarning):

This comment has been minimized.

@pganssle

pganssle Oct 27, 2018

Member
Suggested change Beta
with pytest.raises(DeprecationWarning):
with pytest.warns(DeprecationWarning):

This comment has been minimized.

@smenon8

smenon8 Oct 27, 2018

Contributor

Oops. saw this now. I did not realize this. I am using pytest.deprecated_call() instead which I believe is equivalent.

@pganssle pganssle force-pushed the smenon8:deprecate-requires branch from dfca6f8 to 7bfffe2 Oct 27, 2018

smenon8 added some commits Oct 27, 2018

Deprecate the requires keyword
For runtime dependencies, install_requires should be used. For build
dependencies, a PEP 518-compliant `pyproject.toml` should be used.

Other dependencies can use extra requirements.

@pganssle pganssle force-pushed the smenon8:deprecate-requires branch from 7bfffe2 to 717de83 Oct 27, 2018

PyPA Sprint Weekend at Bloomberg (2018) automation moved this from Submitted PRs to Approved PRs Oct 27, 2018

@pganssle

This looks great to me, @smenon8. Thanks for doing this!

@benoit-pierre @jaraco I have reworded the deprecation warning on this, do you want to take a look and what you think?

Show resolved Hide resolved setuptools/config.py
@jaraco

jaraco approved these changes Oct 29, 2018

@pganssle pganssle merged commit a50f09b into pypa:master Oct 29, 2018

5 checks passed

codecov/patch 100% of diff hit (target 81.45%)
Details
codecov/project 81.47% (+0.02%) compared to 29f9cb0
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

PyPA Sprint Weekend at Bloomberg (2018) automation moved this from Approved PRs to Merged PRs Oct 29, 2018

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