Ticket 21798 #2428

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

PirosB3 commented Mar 13, 2014

No description provided.

tests/field_defaults/tests.py
@@ -15,3 +17,15 @@ def test_field_defaults(self):
self.assertIsInstance(a.id, six.integer_types)
self.assertEqual(a.headline, "Default headline")
self.assertTrue((now - a.pub_date).seconds < 5)
+
+ def test_auto_now_and_auto_now_add_raise_warning(self):
@PirosB3

PirosB3 Mar 13, 2014

Contributor

Strange, looks like there are no warnings captured when running all the test suite. So this test fails.
When running the test on it's own, everything passes.
Any suggestions?

@mxsasha

mxsasha Mar 13, 2014

Member

You're probably hitting the same issue as in https://code.djangoproject.com/ticket/22130#comment:9

@PirosB3

PirosB3 Apr 10, 2014

Contributor

This is no longer an issue as we are not testing warining, but field checks. All tests pass

@timgraham

timgraham May 13, 2014

Owner

warning -> error

@timgraham

timgraham May 13, 2014

Owner

this test would probably be more appropriate in tests/invalid_models_tests/test_ordinary_fields.py

@PirosB3

PirosB3 May 15, 2014

Contributor

thanks will change

Member

mxsasha commented Mar 15, 2014

The code does not take the combination of defaultand auto_now(_add) into account, which we should also not allow in the future, as it is equally invalid.

Also, there are no tests at all in the patch.

Contributor

PirosB3 commented Mar 15, 2014

Hi @erikr
Thanks for the comments.

Regarding tests: I has removed the tests because you said we shouldn't be testing warnings. what would you suggest to test here?

Regarding default: I will add this now. To what should I give more priority to?
I was thinking default > auto_now > auto_now_add

Contributor

PirosB3 commented Mar 15, 2014

Hi @erikr

Based on the comments obtained on IRC. I should not change the priority of the fields at the moment. I have added a DeprecationWarning for all three parameters now (default, auto_now, auto_now_add)
I have also added the documentation for my changes, and the unit tests.

django/db/models/fields/__init__.py
+ # fields. The use of more than one of these fields together
+ # will trigger a DeprecationWarning
+ mutually_exclusive_options = [auto_now_add, auto_now, kwargs.get('default')]
+ if len(filter(lambda o: o not in [None, False], mutually_exclusive_options)) > 1:
@PirosB3

PirosB3 Mar 15, 2014

Contributor

If this syntax understandable?

@mxsasha

mxsasha Mar 15, 2014

Member

At first sight it could be simplified to:

[option not in [None, False] for option in mutually_exclusive_options].count(True)

But I have a feeling even more simplification might be possible.

Note that we can't simply cast default to bool, which would make this even simpler, as some valid dates evaluate to false: https://mail.python.org/pipermail/python-ideas/2014-March/026446.html

docs/ref/models/fields.txt
+ The fields ``auto_now_add``, ``auto_now`` and ``default`` are mutually exclusive.
+ Combining of any of the three is deprecated. ``auto_now`` takes
+ precedence over ``auto_now_add``, ``auto_now_add`` takes precedence
+ over ``default``.
@mxsasha

mxsasha Mar 15, 2014

Member

You can use .. deprecated:: 1.7 for this. Also, any deprecation should be added to the deprecation timeline, and the release notes.

Contributor

PirosB3 commented Mar 15, 2014

Added deprecated tag and refactored enabled_options.

django/db/models/fields/__init__.py
+ # will trigger a DeprecationWarning
+ mutually_exclusive_options = [auto_now_add, auto_now, kwargs.get('default')]
+ enabled_options = [option not in [None, False]
+ for option in mutually_exclusive_options].count(True)
@PirosB3

PirosB3 Mar 15, 2014

Contributor

I split it up for readability. Looks much nicer now! do you agree?

@timgraham

timgraham Apr 8, 2014

Owner

do we need the list iteration? would mutually_exclusive_options.count(True) work?

@PirosB3

PirosB3 Apr 9, 2014

Contributor

Hi @timgraham I just checked list.count and unfortunately count(True) will not work against truthy values but only against values that == True.

ipdb> p mutually_exclusive_options
[False, False, <function now at 0x102fb2050>]
ipdb> p mutually_exclusive_options.count(True)
0

Any suggestions?

django/db/models/fields/__init__.py
+ enabled_options = [option not in [None, False]
+ for option in mutually_exclusive_options].count(True)
+ if enabled_options > 1:
+ warnings.warn("The fields auto_now, auto_now_add and default "
@timgraham

timgraham Apr 8, 2014

Owner

fields -> options
comma after auto_now_add

django/db/models/fields/__init__.py
+ warnings.warn("The fields auto_now, auto_now_add and default "
+ "are mutually exclusive. Combining of any of the "
+ "three is deprecated. auto_now takes precedence "
+ "over auto_now_add, auto_now_add takes precedence "
Owner

timgraham commented Apr 8, 2014

Actually, I think we should just add the check and forget the deprecation warning. There are ways to ignore he check, so it wouldn't be backwards incompatible anyway.

django/db/models/fields/__init__.py
@@ -1074,15 +1074,9 @@ def formfield(self, **kwargs):
return super(CommaSeparatedIntegerField, self).formfield(**defaults)
-class DateField(Field):
+class BaseChronologicalField(Field):
@jarshwah

jarshwah Apr 10, 2014

Member

I don't like the name BaseChronologicalField. It seems like the name is trying to be too clever, and no longer resembles the classes that derive from it.

Perhaps DateTimeCheckMixin or something similar could be used instead?

Contributor

PirosB3 commented Apr 11, 2014

hi @jarshwah thanks for the comments. I refactored BaseChronologicalField into a mixin. This added a little bit of duplication (DateField and TimeField have same constructors) but it does separate concerns better. All the validation is in DateTimeCheckMixin. Please let me know what you think.

docs/ref/models/fields.txt
+ Combining of any of the three is deprecated. ``auto_now`` takes
+ precedence over ``auto_now_add``, ``auto_now_add`` takes precedence
+ over ``default``.
+
@freakboy3742

freakboy3742 Apr 16, 2014

Owner

Not sure if deprecation is the right documentation approach here. We aren't actually changing anything - we're just drawing attention to the fact that there are configuration options that have unpredictable behaviour. I'd be inclined to include this as a normal paragraph inline with the others.

docs/ref/checks.txt
@@ -66,6 +66,7 @@ Fields
* **fields.E134**: ``max_digits`` must be greater or equal to ``decimal_places``.
* **fields.E140**: FilePathFields must have either ``allow_files`` or ``allow_folders`` set to True.
* **fields.E150**: GenericIPAddressFields cannot accept blank values if null values are not allowed, as blank values are stored as nulls.
+* **fields.W151**: The options auto_now, auto_now_add, and default are mutually exclusive. Only one of these options must be present.
@freakboy3742

freakboy3742 Apr 16, 2014

Owner

W151 or E151? I don't think it's out of the question to say that if you're specifying more than one of these, you have an error you need to fix, because the behaviour will be unpredictable. This would only be a warning if the app can continue to operate. Strictly, this is true - after all, you can specify all three - but you really shouldn't. Having this as W151 says "You can do this if you want to, but we suggest you don't", rather than "No, Don't do this".

@PirosB3

PirosB3 Apr 17, 2014

Contributor

Correct! changing W151 to E151. Just one concern: won't this break existing implementations? My reasoning behind using Warning instead of Error is because we give users some time to change their field options (if necessary).

@freakboy3742

freakboy3742 Apr 18, 2014

Owner

Well, if they're using this "feature", their implementation is already
broken - they just don't know it. We don't need to provide a graceful exit
path for behaviour that is known to be broken right now - this is a case of
alerting the user "oh crap - we didn't know you were going to try to do
that - stop it now!".

On Thu, Apr 17, 2014 at 11:52 PM, Daniel Pyrathon
notifications@github.comwrote:

In docs/ref/checks.txt:

@@ -66,6 +66,7 @@ Fields

  • fields.E134: max_digits must be greater or equal to decimal_places.
  • fields.E140: FilePathFields must have either allow_files or allow_folders set to True.
  • fields.E150: GenericIPAddressFields cannot accept blank values if null values are not allowed, as blank values are stored as nulls.
    +* fields.W151: The options auto_now, auto_now_add, and default are mutually exclusive. Only one of these options must be present.

Correct! changing W151 to E151. Just one concern: won't this break
existing implementations? My reasoning behind using Warning instead of
Error is because we give users some time to change their field options (if
necessary).


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/2428/files#r11738824
.

docs/ref/checks.txt
@@ -66,6 +66,7 @@ Fields
* **fields.E134**: ``max_digits`` must be greater or equal to ``decimal_places``.
* **fields.E140**: FilePathFields must have either ``allow_files`` or ``allow_folders`` set to True.
* **fields.E150**: GenericIPAddressFields cannot accept blank values if null values are not allowed, as blank values are stored as nulls.
+* **fields.E151**: The options auto_now, auto_now_add, and default are mutually exclusive. Only one of these options must be present.
@timgraham

timgraham May 13, 2014

Owner

Add `` around auto_now, auto_now_add, and default (similar to E140 above)

docs/ref/models/fields.txt
@@ -477,6 +477,9 @@ The default form widget for this field is a
and a shortcut for "Today". Includes an additional ``invalid_date`` error
message key.
+The fields ``auto_now_add``, ``auto_now`` and ``default`` are mutually exclusive.
@timgraham

timgraham May 13, 2014

Owner

The fields -> The options
add comma after auto_now

tests/field_defaults/tests.py
+ )
+ check = field.check()
+ self.assertEqual(len(check), 1)
+ self.assertEqual(check[0].id, expected.id)
@timgraham

timgraham May 13, 2014

Owner

should probably do something like self.assertEqual(errors, expected) like the other tests in test_models.py, otherwise there is not much use in constructing an Error only to use only the ID.

@PirosB3

PirosB3 May 15, 2014

Contributor

Yes will change now

django/db/models/fields/__init__.py
+ def _check_mutually_exclusive_options(self):
+ # auto_now, auto_now_add, and default are mutually exclusive
+ # options. The use of more than one of these options together
+ # will trigger a DeprecationWarning
@timgraham

timgraham May 13, 2014

Owner

"a DeprecationWarning" -> "an Error"

Owner

timgraham commented May 13, 2014

When you update this, please go ahead and squash your commits and follow our commit message guidelines. Thanks!

django/db/models/fields/__init__.py
+ # will trigger a DeprecationWarning
+ mutually_exclusive_options = [self.auto_now_add, self.auto_now,
+ self.has_default()]
+ enabled_options = [option not in [None, False]
@Stranger6667

Stranger6667 May 13, 2014

Contributor

I think that such iterables like [None, False] should be tuples like (None, False), because it's less memory consuming and faster a bit. Anyway, it is not very important, because of small container size.

Fixed #21798 -- Added check for DateTime mutually exclusive options
Added DateTimeCheckMixin to avoid the use of default, auto_now, and
auto_now_add options together. Added the fields.E151 Error that is raised
if one or more of these options are used together.
Contributor

PirosB3 commented May 16, 2014

hi @timgraham I did the last fixes and squashed. Let me know if something needs updating

Owner

timgraham commented May 16, 2014

merged in cb15231, thanks.

@timgraham timgraham closed this May 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment