AUDIT: r15205 - gnucash/trunk - Load and store a commodity's KVP-frame (IFF it's non-empty).

[Derek Atkins]

I'll just point out a couple of things:

1) This patch doesn't actually change any data formats, because there's
   no data in those fields.  So without additional changes (which I agree
   should not get backported), there's no chance of a data format change.
2) Your proposed change makes it WORSE, because it no longer preserves
   data!  It would happly read in the KVP data but then NOT SAVE IT BACK
   OUT!   I think that would be even worse than tossing chunks.

Keep in mind that just making the XML parser not blow chunks on
unrecognized tags isn't sufficient -- we also need to re-write out
those unrecognized tags or we destroy data.   Reading and silently
discarding is worse than erroring out.

So, I argue that this change still is safe because it's effectively
unused code, but it would let us add additional information in a 2.2
release and not destroy that data in 2.0, which is why I think we should
backport this change.

-derek

Quoting Chris Shoemaker <c.shoemaker at cox.net>:

>
> I don't think this should be back-ported.  At least not the whole
> thing.  I don't mind backporting the first and last hunks, but I'm
> against breaking backward compatibility of the datafile within a
> stable branch.
>
> In fact, even between major releases I think these types of changes
> should only be made if there's enough of them to warrant it.
>
> Also, I'd prefer to see the proposed feature implemented _before_
> breaking compatibilty, not the other way around.
>
> Now, if someone wanted to make it so that our xml reader didn't croak
> on unrecognized tags, that would make this kind of change easier.
>
> So, can we please comment out hunks 2 and 3 with a comment about
> re-enabling them when we're willing to break backward compatibility?
>
> -chris
>
>> Property changes on: gnucash/trunk
>> ___________________________________________________________________
>> Name: svk:merge
>>    - 
>> 3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/branches/swig-redo:802
>> 3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/trunk:943
>> d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk:13679
>> d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk2:13366
>>    + 
>> 3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/branches/swig-redo:802
>> 3889ce50-311e-0410-a464-f059747ec5d1:/local/gnucash/trunk:943
>> d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk:13699
>> d2ab10a8-8a95-4986-baff-8d511d9f15b2:/local/gnucash/trunk2:13366
>>
>> Modified: gnucash/trunk/src/backend/file/gnc-commodity-xml-v2.c
>> ===================================================================
>> --- gnucash/trunk/src/backend/file/gnc-commodity-xml-v2.c	2006-12-11 
>> 22:28:36 UTC (rev 15204)
>> +++ gnucash/trunk/src/backend/file/gnc-commodity-xml-v2.c	2006-12-12 
>> 02:51:37 UTC (rev 15205)
>> @@ -58,6 +58,7 @@
>>  #define cmdty_get_quotes     "cmdty:get_quotes"
>>  #define cmdty_quote_source   "cmdty:quote_source"
>>  #define cmdty_quote_tz       "cmdty:quote_tz"
>> +#define cmdty_slots          "cmdty:slots"
>>
>>  xmlNodePtr
>>  gnc_commodity_dom_tree_create(const gnc_commodity *com)
>> @@ -66,8 +67,11 @@
>>      const char *string;
>>      xmlNodePtr ret;
>>      gboolean currency = gnc_commodity_is_iso(com);
>> +    xmlNodePtr kvpnode =
>> +      kvp_frame_to_dom_tree(cmdty_slots,
>> +			    qof_instance_get_slots(QOF_INSTANCE(com)));
>>
>> -    if (currency && !gnc_commodity_get_quote_flag(com))
>> +    if (currency && !gnc_commodity_get_quote_flag(com) && !kvpnode)
>>        return NULL;
>>
>>      ret = xmlNewNode(NULL, BAD_CAST gnc_commodity_string);
>> @@ -108,6 +112,10 @@
>>        if (string)
>>  	xmlAddChild(ret, text_to_dom_tree(cmdty_quote_tz, string));
>>      }
>> +
>> +    if (kvpnode)
>> +      xmlAddChild(ret, kvpnode);
>> +
>>      return ret;
>>  }
>>
>> @@ -159,6 +167,12 @@
>>  	gnc_commodity_set_quote_source(com, source);
>>          xmlFree (string);
>>      }
>> +    else if(safe_strcmp((char*)node->name, cmdty_slots) == 0)
>> +    {
>> +      /* We ignore the results here */
>> +      dom_tree_to_kvp_frame_given(node,
>> +				  qof_instance_get_slots(QOF_INSTANCE(com)));
>> +    }
>>      else
>>      {
>>          struct com_char_handler *mark;
>>
>> _______________________________________________
>> gnucash-changes mailing list
>> gnucash-changes at gnucash.org
>> https://lists.gnucash.org/mailman/listinfo/gnucash-changes
> _______________________________________________
> gnucash-devel mailing list
> gnucash-devel at gnucash.org
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>



-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available