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

Adds deprecated function to track deprecations #3404

Closed
wants to merge 3 commits into from

Conversation

carltongibson
Copy link
Collaborator

Fixes #3372.

Here's a first pass at a deprecated function.

There was only the one outstanding case, searching for uses of warnings.warn, in the pagination code. I've applied the function there.

  • Is the rough outline acceptable?
  • Happy for it to live in compat.py? (Other option seems to be rest_framework.__init__.py)
  • Do we need tests here?

@carltongibson
Copy link
Collaborator Author

Hmmm. Tests ran at home... :-) I'll have a look at the build.

def deprecated(since, message):
current_version = [int(i) for i in VERSION.split('.')]

assert current_version[0] == since[0], "Deprecated code must be removed before major version change. Current: {0} vs Deprecated Since: {1}".format(current_version[0], since[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be > here ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might have been confused by the since argument. Maybe it should be named target_version or something similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps deprecated_from_version — I've no objection to long variable names.

@carltongibson
Copy link
Collaborator Author

OK. I'll add tests around deprecated to satisfy the coverage requirement.

I can't understand the lint failure. It says run isort, but that's what I've done...

(drf)~/Documents/Django-Stack/django-rest-framework (deprecated)$ isort --recursive .
(drf)~/Documents/Django-Stack/django-rest-framework (deprecated)$ ./runtests.py --lintonly
Running flake8 code linting
flake8 passed
Running isort code checking
isort passed

Yet on Travis it fails. Confused...

@tomchristie
Copy link
Member

Def same version of isort on each? (travis says isort==3.9.6)

@carltongibson
Copy link
Collaborator Author

@tomchristie That'll probably be it.

I'll tidy this up later now.

In general, are we happy with the approach?

@tomchristie
Copy link
Member

Can't 100% decide if the indirection is a benefit or not, but prob ok.
We might want to include a stand bit of messaging "This behavior was put on the deprecation path in ***. It will be removed in version ***.

@carltongibson
Copy link
Collaborator Author

Re tests:

$ ./runtests.py --lintonly
Running flake8 code linting
flake8 passed
Running isort code checking
isort passed
$ pip freeze | grep isort
isort==3.9.6

... so still confused. (Will work it out.)

@carltongibson
Copy link
Collaborator Author

Can't 100% decide if the indirection is a benefit or not, but prob ok.

I think the key is that you only have to mark it deprecated once, rather than remember for each update. @xordoquy can probably say best, since he's on the sharp-end

We might want to include a stand bit of messaging "This behavior was put on the deprecation path in **. It will be removed in version **.

OK. I'll add this.

@carltongibson
Copy link
Collaborator Author

Quick check before I get back to this:

Once all the boxes are ticked, do we want this? (If not what of #3372? — is "search for DeprecationWarning and delete/update" enough of a system?)

@carltongibson
Copy link
Collaborator Author

@xordoquy What do you think here? Is this helpful for what you had in mind in #3372, or not? (I'm happy to finish it off, but am not attached to it, if you don't like it.)

Let me know.

@carltongibson
Copy link
Collaborator Author

Closing in favour of #3478

@carltongibson carltongibson deleted the deprecated branch August 29, 2017 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants