-
Notifications
You must be signed in to change notification settings - Fork 347
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
#596 MarkInfo deprecation #603
#596 MarkInfo deprecation #603
Conversation
@@ -378,10 +378,10 @@ def _django_db_marker(request): | |||
This will dynamically request the ``db`` or ``transactional_db`` | |||
fixtures as required by the django_db marker. | |||
""" | |||
marker = request.keywords.get('django_db', None) |
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 two places like this bump the requirements to pytest >= 3.6
- I think it would be easy enough to have code branch for pytest < 3.6
to maintain existing compatibility if that feels like a blocker to this PR.
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.
No, I think requiring pytest 3.6 is fine.
if marker: | ||
validate_django_db(marker) |
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.
I'm not 100% happy with the naming, but I didn't think of anything better.
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.
What naming do you mean?
marker
here, or transaction
below?
As for transaction
it could be named transactional
mabye?
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.
I was thinking validate_django_db(…)
as it gets a value, and may happen to blow up, but it feels more ok to me now
@@ -11,6 +11,7 @@ _build | |||
/.coverage | |||
/htmlcov/ | |||
.cache | |||
.pytest_cache/ |
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.
This was missing, so added it in a separate commit.
Codecov Report
@@ Coverage Diff @@
## master #603 +/- ##
=======================================
Coverage 91.92% 91.92%
=======================================
Files 33 33
Lines 1660 1660
Branches 143 143
=======================================
Hits 1526 1526
Misses 95 95
Partials 39 39
Continue to review full report at Codecov.
|
Merged with changelog entry in 8d548e7. |
Don't you see other errors then btw? Tried it changing the
Looks like this one is coming from Python (3.6.5) itself?! |
I didn't encounter that in the environment I was in. It is strange that the patch is 7 years old - it is included in at least the 3.4+ branches on GitHub, but not 2.7, so I think something is up with the environment like 2.7 files are being used. |
Hi guys, just an FYI; This PR turned out to be breaking my builds, as it replaces the pytest required version from (the documented, still in README.md) 2.9 up to 3.6. |
@nirizr See #603 (comment) - we/I've decided that it's OK to bump it, that's also why it became |
Here is a quick go at removing the deprecation warning mentioned in #596 so tests won't break if running with
-W error
.