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

Deprecations for 2.0 #792

Merged

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Oct 18, 2017

  • Rename Filter.name => Filter.field_name
  • Deprecate Meta.together
  • Deprecation note for FILTERS_STRICTNESS, Meta.strict, FilterSet.strict

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Yep. I think that'll do.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 18, 2017

Ach. Why do you forsake my python 2???

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 18, 2017

I am perplexed - this does not fail locally for me.

@carltongibson
Copy link
Owner

Give me a sec. I will give it a run.

@carltongibson
Copy link
Owner

Right, I'm perplexed too.

Travis has two failures. (One for each test).

I'm getting a failure just for the second.

................................................F..........................................................s..x...ss..x..x..........................................................................................................................................................................................................................s.........................sss..................................................................................
======================================================================
FAIL: test_filter_strictness (tests.test_deprecations.DeprecatedSettingsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/carlton/Documents/Django-Stack/django-filter/tests/test_deprecations.py", line 37, in test_filter_strictness
    self.assertEqual(len(w), 1)
AssertionError: 0 != 1

----------------------------------------------------------------------
Ran 451 tests in 1.274s

FAILED (failures=1, skipped=7, expected failures=3)

Given that it's exactly the same as the first, that doesn't make much sense to me.

@carltongibson carltongibson added this to the Version 1.1 milestone Oct 18, 2017
@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 18, 2017

I'll have to come back to this tomorrow - I can't figure out what's going on :/

@rpkilby rpkilby changed the title [1.1] Deprecate the 'Meta.together' option [1.1] Deprecate 'Meta.together', 'FILTERS_STRICTNESS', Filter.name moved to Filter.field_name Oct 18, 2017
@rpkilby rpkilby changed the title [1.1] Deprecate 'Meta.together', 'FILTERS_STRICTNESS', Filter.name moved to Filter.field_name [1.1] Deprecations for 2.0 Oct 18, 2017
@rpkilby rpkilby force-pushed the deprecate-together-option branch 7 times, most recently from 300a808 to 37aea43 Compare October 18, 2017 22:47
@rpkilby rpkilby force-pushed the deprecate-together-option branch 2 times, most recently from 4a7ee18 to 3d67c18 Compare October 18, 2017 22:55
@rpkilby rpkilby force-pushed the deprecate-together-option branch from 9a0f9d7 to d8b5d54 Compare October 18, 2017 23:17
@codecov-io
Copy link

codecov-io commented Oct 18, 2017

Codecov Report

Merging #792 into develop will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #792     +/-   ##
==========================================
+ Coverage    97.45%   97.56%   +0.1%     
==========================================
  Files           15       15             
  Lines         1220     1234     +14     
==========================================
+ Hits          1189     1204     +15     
+ Misses          31       30      -1
Impacted Files Coverage Δ
django_filters/filterset.py 100% <100%> (ø) ⬆️
django_filters/filters.py 97.38% <100%> (+0.09%) ⬆️
django_filters/utils.py 99.1% <0%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9fcde4...ded646b. Read the comment docs.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 18, 2017

I'm still not entirely sure why the tests were failing on CI and not locally, but the issue was related to a bad stacklevel on a few of the warnings.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 18, 2017

@carltongibson, so I think this is ready, but I'm not sure if we want to include this in 1.1

In short, I don't see a clean migration path that accommodates both the old FilterSet strictness behaviors and the proposed strict handling in the views. I also don't think it's really worth the effort even try, given that it would require a bunch of tests that are going to be short lived.

If we merge this, there will be no migration path for strictness, so v1.1 will always raise warnings for the user. Alternatively, this can be merged into a v1.1.1 or v1.2 deprecation/migration release. What do you think?

@carltongibson
Copy link
Owner

@rpkilby:

... strictness ... there will be no migration path for strictness, so v1.1 will always raise warnings for the user.

Either of:

  • Let people silence the warning.
  • Merge the rest but just DON'T add the deprecation warning for strictness: Folks are just going to have to notice this in the migration guide when upgrading. (I think that's OK — it's the smoothest migration path — no reason it has to be totally frictionless if that's not OK.)

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 19, 2017

The latter seems preferable. The release notes can make mention of the strictness deprecations instead.

@rpkilby rpkilby force-pushed the deprecate-together-option branch from d8b5d54 to ded646b Compare October 19, 2017 06:59
@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 19, 2017

Alright, this should be done. I've removed the strictness warnings, but left the deprecation note.

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK. This is good. Well done.

@carltongibson carltongibson merged commit 6d88552 into carltongibson:develop Oct 19, 2017
carltongibson pushed a commit that referenced this pull request Oct 19, 2017
* Deprecate the 'Meta.together' option

* 'Filter.name' => 'Filter.field_name'

* Add strictness migration note
@rpkilby rpkilby deleted the deprecate-together-option branch October 19, 2017 09:26
@rpkilby rpkilby changed the title [1.1] Deprecations for 2.0 Deprecations for 2.0 Jan 27, 2018
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