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

Drop use of six.u #1517

Merged
merged 2 commits into from Oct 23, 2018

Conversation

Projects
None yet
3 participants
@zapstar
Contributor

zapstar commented Oct 23, 2018

Summary of changes

Drop use of six.u in the code. Trivial changes, I thought you folks had more than one occurrence of six.u here.

Closes #1516

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
dist = Distribution()
cmd = ei.easy_install(dist)
tmp_dir = None
with cmd._tmpdir() as tmp_dir_name:

This comment has been minimized.

@pganssle

pganssle Oct 23, 2018

Member

We don't usually test the private interface of these functions directly, it's better to try to hit the relevant code indirectly. _tmpdir seems to have pretty good code coverage already as it's used every time easy_install is called.

This comment has been minimized.

@zapstar

zapstar Oct 23, 2018

Contributor

So no tests required?

This comment has been minimized.

@pganssle

pganssle Oct 23, 2018

Member

Very good instincts to add a test, though.

I suspect there is some way to test this particular functionality by trying to assert that none of the files written into this directory persist when easy_install is done being used, but frankly we're trying to get rid of easy_install anyway, so it's probably not worth spending much time working out clever tests for it.

This comment has been minimized.

@pganssle

pganssle Oct 23, 2018

Member

So no tests required?

Correct, you don't need to add a test for this.

@pganssle pganssle force-pushed the zapstar:master branch from f2c7351 to 0395f68 Oct 23, 2018

@pganssle

This comment has been minimized.

Member

pganssle commented Oct 23, 2018

I have dropped the unnecessary test, this can be merged when CI passes.

@pganssle

This comment has been minimized.

Member

pganssle commented Oct 23, 2018

@zapstar Thanks for your PR, really quick on the draw! Once the CI passes myself or one of the other maintainers will merge it. Feel free to check out other issues in the help wanted label if you want something with a bit more meat to dig into!

@pganssle

This comment has been minimized.

Member

pganssle commented Oct 23, 2018

@benoit-pierre Did you want to merge this? I don't know what you need to see with respect to the rolling builds, so I don't want you to miss it if I do the merge.

@benoit-pierre benoit-pierre merged commit 8c22c3b into pypa:master Oct 23, 2018

5 checks passed

codecov/patch 100% of diff hit (target 81.43%)
Details
codecov/project 81.43% (+0%) compared to 5b90a0d
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
@benoit-pierre

This comment has been minimized.

Member

benoit-pierre commented Oct 23, 2018

It's unrelated to rolling builds support, but yes, assuming setuptools suffered from the same bug, let's see how many builds are triggered by the merge!

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