Merge lp:~mandel/desktopcouch/record_id into lp:desktopcouch

This branch ensures that the record_id is set whenever the +id key is present in the data pass to create a new record.

Ensured that when creating a record with a dict that contains the key '_id' its vlaue is used for record_id

Let's not use "warnings" module. Instead, import logging and "logging.warn(...)", usually.

In this case, though, I would be just as happy with an assertion.

if '_id' in data:
   if record_id is not None:
       assert data['_id'] == record_id
       ...

Maybe drop the "record_id" member and make record_id a @property like record_type. Maybe we can drop all "_id" from code except for the getter.

Do we need a setter property also?

> if '_id' in data:

if data.get('_id', None) is not None: # safer. Explicit {'_id':None} would fail.

> Let's not use "warnings" module. Instead, import logging and
> "logging.warn(...)", usually.
>
> In this case, though, I would be just as happy with an assertion.
>
> if '_id' in data:
> if record_id is not None:
> assert data['_id'] == record_id
> ...
>
> Maybe drop the "record_id" member and make record_id a @property like
> record_type. Maybe we can drop all "_id" from code except for the getter.
>
> Do we need a setter property also?

I agree with the comments, the new implementation uses an assertion. I have implemented the record_id as a property with a getter and a setter. The setter is needed to pass all current tests.

13 + if '_id' in data and record_id is not None:
14 + assert data['_id'] == record_id
15 + elif record_id is not None:
16 + self._data['_id'] = record_id

This would be cleaner with two levels of "if".

if record_id is not None: # Explicit setting, so try to use it.
   if data.get('_id', None) is not None: # Overwrite _id if it is None.
      self._data['_id'] = record_id
   else: # _id already set. Verify that the explicit value agrees.
      assert self._data['_id'] == record_id, "record_id doesn't match value in data."

Note the data.get() and the second parameter to assert .

> 13 + if '_id' in data and record_id is not None:
> 14 + assert data['_id'] == record_id
> 15 + elif record_id is not None:
> 16 + self._data['_id'] = record_id
>
>
> This would be cleaner with two levels of "if".
>
> if record_id is not None: # Explicit setting, so try to use it.
> if data.get('_id', None) is not None: # Overwrite _id if it is None.
> self._data['_id'] = record_id
> else: # _id already set. Verify that the explicit value agrees.
> assert self._data['_id'] == record_id, "record_id doesn't match value in
> data."
>
>
>
> Note the data.get() and the second parameter to assert .

What if the record_id is not None and the data['_id']is indeed None? we could try to put in the db {'_id':None} which will raise an exception.

We would have to do yet another check.

> > 13 + if '_id' in data and record_id is not None:
> > 14 + assert data['_id'] == record_id
> > 15 + elif record_id is not None:
> > 16 + self._data['_id'] = record_id
> >
> >
> > This would be cleaner with two levels of "if".
> >
> > if record_id is not None: # Explicit setting, so try to use it.
> > if data.get('_id', None) is not None: # Overwrite _id if it is None.
> > self._data['_id'] = record_id
> > else: # _id already set. Verify that the explicit value agrees.
> > assert self._data['_id'] == record_id, "record_id doesn't match value
> in
> > data."
> >
> >
> >
> > Note the data.get() and the second parameter to assert .
>
> What if the record_id is not None and the data['_id']is indeed None? we could
> try to put in the db {'_id':None} which will raise an exception.
>
> We would have to do yet another check.
Sorry, I just realized what you meant... override _id. Stupid Manuel!

Looks good to me. Thank you.

Looks good.

Major screw up from my part. The committed patch is wrong (major screw up from my port, I should have added a test!!!)

The if statement should be:

if record_id is not None: # Explicit setting, so try to use it.
   if data.get('_id', None) is None: # Overwrite _id if it is None.
      self._data['_id'] = record_id
   else: # _id already set. Verify that the explicit value agrees.
      assert self._data['_id'] == record_id, "record_id doesn't match value in data."

Otherwise we will always raise an exception since None is never equal to record_id!!

I have made the changes in the same lp branch with an addition of a test case to ensure that next time this does not happen.

Sorry for the screw up!

Manuel