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

Allow schema = None. Deprecate exclude_from_schema #5422

Merged

Conversation

carltongibson
Copy link
Collaborator

The PR allows setting schema = None to exclude APIView subclasses from schema generation.

For function based views use @schema(None).

It deprecates exclude_from_schema on APIView and the exclude_from_schema argument to api_view.

WrappedAPIView.exclude_from_schema = exclude_from_schema
if exclude_from_schema:
warnings.warn(
"The `exclude_from_schema` argument to `api_view` is deprecated. "
Copy link
Member

Choose a reason for hiding this comment

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

"is pending deprecation"


assert should_include == expected

def test_deprecations(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rpkilby I'm getting console noise in the test run from this. (Well from the call site but...)

tests/test_schemas.py ............/Users/carlton/Documents/Django-Stack/django-rest-framework/rest_framework/decorators.py:83: PendingDeprecationWarning: The `exclude_from_schema` argument to `api_view` is deprecated. Use the `schema` decorator instead, passing `None`.
  PendingDeprecationWarning
/Users/carlton/Documents/Django-Stack/django-rest-framework/rest_framework/schemas/generators.py:156: PendingDeprecationWarning: OldFashjonedExcludedView. The `APIView.exclude_from_schema` is deprecated. Set `schema = None` instead
  warnings.warn(msg, PendingDeprecationWarning)
....
tests/test_serializer.py .........................................

Can you advise on the best way to keep that quiet? Ta!

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need to test this. Don't believe that we've done so when deprecating other bits of API.

Copy link
Collaborator Author

@carltongibson carltongibson Sep 14, 2017

Choose a reason for hiding this comment

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

warnings.warn(
"The built in 'rest_framework.filters.FilterSet' is deprecated. "
"You should use 'django_filters.rest_framework.FilterSet' instead.",
DeprecationWarning, stacklevel=2
)

and

@unittest.skipUnless(django_filters, 'django-filter not installed')
def test_backend_deprecation(self):
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
view = FilterFieldsRootView.as_view()
request = factory.get('/')
response = view(request).render()
assert response.status_code == status.HTTP_200_OK
assert response.data == self.data
self.assertTrue(issubclass(w[-1].category, DeprecationWarning))
self.assertIn("'rest_framework.filters.DjangoFilterBackend' is deprecated.", str(w[-1].message))

It's something @rpkilby always puts in. I thought, Why not? 🙂

Copy link
Member

@rpkilby rpkilby Sep 14, 2017

Choose a reason for hiding this comment

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

It looks like pytest.warns isn't recommended for deprecation warnings, as it doesn't capture output.
ach... I completely misread the docs. Either way, the below section sufficiently explains how to test deprecation warnings.

https://docs.pytest.org/en/latest/warnings.html#ensuring-function-triggers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rpkilby OK. Thanks. I read that too but was still getting the noise...

  1. I went with pytest.warns(PendingDeprecationWarning) because (for me) pytest.deprecated_call() failed with DID NOT WARN. No ideas why that wasn't working.
  2. Despite the noise locally for me, this is not appearing in the travis run — so it must be my local setup.

(There is plenty of noise from py.test in travis but I assume that'll disappear with time.)

Lets call this done. Thanks for the input!

@tomchristie I think we're good to go.

Copy link
Member

Choose a reason for hiding this comment

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

Despite the noise locally for me, this is not appearing in the travis run — so it must be my local setup.

No noise when running the tests on my machine, so that would make sense.

@carltongibson carltongibson added this to the 3.6.5 Release milestone Sep 15, 2017
@carltongibson
Copy link
Collaborator Author

Milestoned for 3.6.5. I'll convert that to 3.7 next week and begin putting together a release.

@tomchristie
Copy link
Member

@carltongibson Looks good. 👍

@carltongibson carltongibson merged commit 7b1582e into encode:master Sep 20, 2017
@carltongibson carltongibson deleted the deprecate-exclude_from_schema branch September 20, 2017 09:29
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.

3 participants