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

Fix some usage of deprecated imp module #1798

Merged
merged 1 commit into from
Aug 17, 2019
Merged

Fix some usage of deprecated imp module #1798

merged 1 commit into from
Aug 17, 2019

Conversation

asottile
Copy link
Contributor

@asottile asottile commented Jul 8, 2019

Summary of changes

Partial resolution of #479 -- the other usage is a little more tricky and will require some more clever fixes

Pull Request Checklist

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

@asottile asottile closed this Jul 8, 2019
@asottile asottile reopened this Jul 8, 2019
@asottile
Copy link
Contributor Author

asottile commented Jul 8, 2019

not sure why travis didn't report status -- it is complete and successful 🤷‍♂️

@asottile asottile mentioned this pull request Jul 11, 2019
setuptools/command/install_lib.py Show resolved Hide resolved
@pganssle
Copy link
Member

@asottile I don't have any objections to this, but when running the test suite against master with DeprecationWarning set to raise errors on master, I'm not getting any failures, but coverage.py is saying all these lines are covered. Any idea what's going on there?

@asottile
Copy link
Contributor Author

@asottile I don't have any objections to this, but when running the test suite against master with DeprecationWarning set to raise errors on master, I'm not getting any failures, but coverage.py is saying all these lines are covered. Any idea what's going on there?

depending on the version of pytest you're using (I've only fixed pytest in 5.x) imp will get imported as a side-effect of pytest's initialization (and so imports in user-space code won't trigger that warning) -- this is probably what you're seeing if I had to guess?

@pganssle
Copy link
Member

Hm.. That seems like a huge problem with pytest, that's super annoying.

@asottile
Copy link
Contributor Author

hehe yeah that's why I fixed it 🙃

@asottile
Copy link
Contributor Author

asottile commented Aug 8, 2019

@pganssle @jaraco any updates on this? I think this is ready for merge

Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Sorry to let this languish for so long. I am made mildly uncomfortable by the fact that there's no regression test in this (and thus no automated verification that this fixes the problem), but given the issue with pytest and the fact that we can't really turn on warnings-as-errors yet (see #1823), I think we should just go ahead and merge this.

@mergify mergify bot merged commit 2cd2fdc into pypa:master Aug 17, 2019
@asottile asottile deleted the some_imp branch August 17, 2019 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants