Set a field to None removes it from DB

[lafrech]

I'm using this patch to allow the user to remove a field by setting it to None.

Basically, when an object attribute is set to None, rather than getting a validation error, the value is changed into missing and the field is removed.

It seems to make sense to me and it works on my use case but it's hardly tested, so there may be corner cases I didn't think of.

Is there already a clean way to do that I didn't know about? If not, is it a feature you'd find useful?

[coveralls]

Coverage decreased (-0.05%) to 93.701% when pulling 153aa15 on Nobatek:none_means_missing into 3c5a9cf on Scille:master.

[touilleMan]

The use of None is always slippy given you never know what it really means.

The policy in umongo is to say "None is None", so if you want to set a field to None you must have set allow_none=True in your field config.

If you want to delete a field from a document, then you should use the del keyword 👍

Only exception is the fields that are missing in the database which are represented to the user as None given comparing against a missing object would be to cumbersome (thus we could make the missing value configurable and None by default).

I realize this behavior can cause some trouble, so maybe we should add an special message when the user try to set a field to None and is not allowed to do so ("If you want to delete this field, use the del keyword").

[lafrech]

We've been discussing this today.

If you want to delete a field from a document, then you should use the del keyword

OK, this make sense.

Only exception is the fields that are missing in the database which are represented to the user as None given comparing against a missing object would be to cumbersome (thus we could make the missing value configurable and None by default).

Wouldn't it be better to raise AttributeError / KeyError (i.e. to_raise)?

We see two reasons for this:

• Consistency: If you delete an attribute from a Document, trying to access it yields an error. If you commit then find, you should expect the same behaviour. The Document should not return either an exception or None depending on whether it was saved to / read from database in the meantime.
• None status: As you said, None can hold a meaning, so having None in the Document is not the same as a missing attribute. Returning None if the value is missing beats the point of making a difference between missing and None, doesn't it?

I hope I'm making sense.

In fact, consistency was the reason I created this PR, but I did it the other way around, which was wrong.

If you agree with this, I'd like to close this PR and send a new one making get return an exception.

    def get(self, name, to_raise=KeyError):
        if name not in self._fields:
            raise to_raise(name)
        field = self._fields[name]
        name = field.attribute or name
        value = self._data[name]
        if value is missing:
            if self.partial:
                raise FieldNotLoadedError(name)
            elif field.default is not missing:
                return field.default
            else:
                return to_raise
        return value

Doing this, we keep the default value. That is, assuming I have a field with a default value:

del my_doc.my_attribute
# This prints default_value (no exception)
print(my_doc.my_attribute)

I don't really understand the use of partial but I think the change we propose is coherent with it.

[lafrech]

On second thought, the "consistency" argument is based on the wrong assumption that when deleting an attribute from a doc would yield an Exception, but this is incorrect. It does return None before and after commit/find. In fact, commit has nothing to do with this, so whichever solution is chosen, coherence should be maintained.

Considering the difference between None and missing, after our discussion, I understand your point of view. In most cases, it is practical to get None rather than an Exception and missing is equivalent to None, so you'd rather have this behaviour by default and allow the user to provide another value of his choice for missing fields when he needs disambiguation from None.

I don't have any definitive opinion about this, but in any case, this PR should not be merged.