-
-
Notifications
You must be signed in to change notification settings - Fork 533
Fix bug with incorrectly defactorized dependencies #768 #1057
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
Conversation
New version with improved changes, fixing errors raised with the previous version: tox-dev#899 tox-dev#906
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment, for us to merge it let's make tests a bit more onto the point, and some explanation why the command repetition is needed.
|
||
x = self._replace_if_needed(x, name, replace, crossonly) | ||
# print "getstring", self.section_name, name, "returned", repr(x) | ||
return x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment here is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is not mine, I kept it as it was already in the code.
I would prefer to not having commented code. If you do prefer I can simply remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
assert packages == ["bar"] | ||
|
||
# https://github.com/tox-dev/tox/issues/899 | ||
def test_regression_test__issue_899(self, newconfig): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer a name that explains the edge case comparison, so maintainers don't need to feed the issue in detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll change it.
[testenv] | ||
skip_install = True | ||
sitepackages = True | ||
passenv = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also please remove lines not essential to reproduce the bug, it just makes hard to read the test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll change it.
] | ||
|
||
# https://github.com/tox-dev/tox/issues/906 | ||
def test_regression_test__issue_906(self, newconfig): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll change it.
|
||
x = self._replace_if_needed(x, name, replace, crossonly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have here some explanation of why the repetition of same commands is needed as a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll try to summarize the issue as much as I can.
New version with improved changes, fixing errors raised
with the previous version:
#899
#906
Thanks for contributing a pull request!
If you are contributing for the first time or provide a trivial fix don't worry too
much about the checklist - we will help you get started.
Contribution checklist:
(also see CONTRIBUTING.rst for details)
in message body
<issue number>.<type>.rst
for example (588.bugfix.rst)<type>
is must be one ofbugfix
,feature
,deprecation
,breaking
,doc
,misc
superuser
."CONTRIBUTORS
(preserving alphabetical order)