Skip to content

Drop use of six.u#1517

Merged
benoit-pierre merged 2 commits into
pypa:masterfrom
zapstar:master
Oct 23, 2018
Merged

Drop use of six.u#1517
benoit-pierre merged 2 commits into
pypa:masterfrom
zapstar:master

Conversation

@zapstar

@zapstar zapstar commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

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

Comment thread setuptools/tests/test_easy_install.py Outdated
dist = Distribution()
cmd = ei.easy_install(dist)
tmp_dir = None
with cmd._tmpdir() as tmp_dir_name:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So no tests required?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So no tests required?

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

@pganssle

Copy link
Copy Markdown
Member

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

@pganssle

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

@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
@benoit-pierre

Copy link
Copy Markdown
Member

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants