GNOME.org
 

Re: make check doesn't work at gsoc2013

Hi Daniel.

Yay, I finally went through the code!  Sorry that it took so long; was at GUADEC, had to replace my computer, etc; the branch has lots of different changes, though, so I needed to sit down and follow it all.  Sorry if I've blatantly misunderstood anything, but I want to get this feedback out now rather than worrying about it any longer :D. The short version is first, details further below inline.  Know that I'm not opposed to changing things, but I prefer to make the smallest changes necessary.


Regarding larger API changes

- I'd like to retain API compatibility with json-glib, GObject, and GObjectClass, so I'd rather not alter method signatures in GXmlSerializable

- the added default implementations for GXmlSerializable's serialize_property and deserialize_property seem similar to GXmlSerialization's corresponding methods; any reason to duplicate it there?  Also, GXmlSerializable's new serialize () and deserialize () overlap with the existing GXmlSerialization's serialize_object () and deserialize_object ().  Can you elaborate on your motivations for wanting functionality inside GXmlSerializable instead of GXmlSerialization?  (Sorry if you've articulated it clearly before and I've missed it)

- the resulting XML output seems like it might be different (is that intentional?  with properties as attributes instead of child elements?)

- I have questions about some of the actual logic changes/differences (further below, inline with your message)



Regarding additions:

- the signals for serialize_unknown_property and deserialize_unknown_property seem neat; is the idea to use that in place of SerializationErrors/SerializableErrors? I see they're actually emitted after successfully {,de}serialization; can you describe the use cases?

- the HashTable for ignored_serializable_properties seems useful; the same ability is I think available by overriding find_property () and list_properties (), but that requires more work; when and how would a user use ignored_serializable_properties? (There might be an example in the code you've shared with me, but having it as a doc comment would be useful :)

- the HashTable for unknown_serializable_property seems useful for providing uninterrupted access to which properties aren't spec'd for that object class. Currently, a user would find out when they call gxml_serialization_serialize_object () and it throws a SerializationError; are you trying to avoid that?  (I see you still use SerializableError when using string_to_gvalue, though)

- properties for setting node names/property nicks might be neat; is it just to have more control over what the resulting XML looks like?



To get stuff merged,

- can new features remain compatible with json-glib's approach and API (basically relying on GObject and GObjectClass behaviour where possible)

- can new features be broken up and considered separately?
  - e.g. the signals, the hashtables the node name stuff, default implementation logic changes (does it need to relocate functionality?), gxml_element_append_content
 
- if you require an API that more closely mimics .NET's XmlSerializer and not json-glib, is it possible to do it on top of GXmlSerializable and GXmlSerialization, instead of trying to replace them?



On Tue, Aug 13, 2013 at 1:02 PM, Daniel Espinosa <esodan gmail com> wrote:


El ago 7, 2013 3:51 a.m., "Richard Schwarting" <aquarichy gmail com> escribió:


>
> Hi Daniel.
>
> Thanks again for the work.  So, I haven't compiled and run your code in the serialization branch, but after reading it, I have some questions, to help me better understand what your goal is.
>
> *** DomNode.vala
> - is there any reason that you're defaulting _str to "DomNode" for to_string ()?
>   - in theory, it gets built anew each time.  (In fact, I'm not sure whether we need the _str field anymore.)
>

Nop. I don't remember to change it. I agree to remove _str too.

But I start to think that Node must be an interface with virtual methods as default implementation.

Yeah, Node is basically acting like an interface right now anyway.

> *** Element.vala
> - any particular reason you're changing from set_content to
>   add_content when we're setting the content of an Element? 
>

Yes. set_content deletes my added nodes and replace them with the text set and that's I want to avoid. I think we need to add a method to replace content or set text in a node in order to replace its nodes with text or just add text with no child nodes remove.

My documentation for the content property is wrong.  I claimed that you'd get a stringified version of its contents (nodes and all) but that's false, it only returns a string of the Text descendants.  Whoops.

I would like gxml_node_set_content to continue to replace the content of a Node.  Would you accept a new method like gxml_node_append_content () or gxml_node_append_text?  It would work like:
mynode.child_nodes.append_child (doc.create_text_node ("blah"));

> *** NodeList.vala
> - NodeListIterator.advance () should not need to explicitly call
>   .is_empty () because the calling code, GenericNodeListIterator, does
>   that itself.  NodeListIterator.advance has protected visibility, so
>   nothing outside will be calling it either.
>

You're right. I forget to remove it when I try to fix a crash. If change it just need to test against my branch and unit tests I want to add.

> *** Serializable.vala
>
> The interface for Serializable was modelled off of ebassi's
> serializable interface for JSON.  The names of the interface methods
> and their signatures are intended to resemble those in GObject and
> GObjectClass.
>
> I notice that you've explicitly changed the names of some methods, and seemed to move some class methods of Serialization into Serializable methods.  Any particular reason why?
>
Yes. Setting names to GObjectClass seems to me unclear about it purposes, but the main reason is because I added the possibility to skip properties to be serializable and then is just get properties.

serializable branch tries to implement .Net's Xml.Serializer  while keeps your actual implementation unchanged, may be by adding a new class or new properties to Serialization class to use one method or other.  For now just Serializable is implemented, when Unit Tests are fixed I've continue on Serialization class and merge both methods.

That way, whether someone uses JSON or XML, it'll be essentially the same API.  (Perhaps in the future, there can be a common serialization API or something in GNOME.)  I need to review what json-glib has done recently, as we might differ anyway.

Consequently, for providing an interface like .NET's XML Serializer (http://msdn.microsoft.com/en-us/library/system.xml.serialization.xmlserializer.aspx is the one?  I reviewed something like that last year when I started work on Serialization :D), can we leave Serializable as a generally empty interface mimicing json-glib and let a separate class/interface implement/extend from it? 

So instead of changing GXmlSerializable (which has the goal of matching json-glib's Serializable), add something like GXmlNetSerializable which as an interface extends GXmlSerializable (or as a class implements it, or just uses it).

> I'm going to read through the logic now and see what motivates all the new methods.
>

My objective is to have .Net equivalent functionality into GXml, to port one of my projects to Vala. If you want to see your self I can share you some XML files and you can download OpenSclConfigurator[1] app witch uses OpenScl library to de/serialize XML files to class objects, through .Net Xml.Serializer class.

[1] sourceforge.net/projects/ opensclconfigurator

Is the problem that GXmlSerializable and GXmlSerialization don't go far enough for you, or is it more that you want an API that mirrors .NET's rather than json-glib's?  I'm not opposed to adding default logic to GXmlSerializable, but I don't want it to be redundant with what's already available in GXmlSerialization, especially if it can be small changes to GXmlSerialization instead of large changes to GXmlSerializable.

I note that so far,
- GXmlSerialization hasn't changed much (with some methods moving to GXmlSerializable).
- GXmlSerializable
  - adds signals: deserialize_unknown_property, serialize_unknown_property
    - neat; can you describe a couple use cases?

  - serialize ()
  - deserialize ()
    - is this redundant with gxml_serialization_{de,}serialize_object ()?  Is there any particular reason you want to attach this to the class?  One of the benefits of having it in GXmlSerialization is that it handles GObjects and Serializable objects without the user having to worry.

  - deserialize_property () adds default impl
    - what does the existing gxml_serialization_deserialize_property () class method lack that you want to add new logic here, or are you just mostly moving it over?
    - should the case where it deserialize's from a GXmlAttr be mutually exclusive with the preceding cases?
    - you check if you can transform a GXmlNode to prop.value_type; is there some reason GValue would support that? For the future?
    - signal emission :)

  - serialize_property:
    - are you preferring serializing properties into XML attributes instead of as independent elements?
    - the existing gxml_serialization_serialize_property () seems to handle a couple more cases (I know the code is kind of messy; I should clean that up real quick :D)  Is there any reason why the logic is different
    - signal emission :)

  - find_property_spec
    - body mostly to support the property blacklist ignored_serializable_properties, I guess?
    - it lowercases the property name, perhaps mostly for ignored_serializable_properties, but if a property for some reason has a capital in it, GObjectClass's find_property is actually case sensitive, and it won't find it any more. :(

  - list_serializable_properties
    - body mostly to support property blacklist, I guess :)

  - get_property_value
    - I was also tempted to just return a string, but still used GValue ref param, to match json-glib and GObject, which uses GValues to be type agnostic. Matching g_object_get_property has been important so we can just override it only in cases where we have custom property handling beyond GObject (e.g. a private field or a custom collection).  Elseiwse, GXml's serialization has wanted to just rely on GObject's only property handling to avoid adding more code.
    - supports ignored_serializable_properties

  - set_property_value
    - similar desire to match g_object_set_property () so that we'll only override it when necessary
    - supports ignored_serializable_properties
   
  - adds some properties
    - serializable_node_name and serializable_property_use_nick
      - is there any particular reason you want to be able to customise the node names?

    - ignored_serializable_properties
      - is this mostly for implementers to be able to selectively ignore some properties?  In theory, that's a capability that the existing gxml_serializable_list_properties/gxml_serializable_find_property provides, though a bit more convoluted.  Are you mostly wanting to add a simpler/more automatic way to do it?  That seems useful.

    - unknown_serializable_property
      - should this be unknown_serializable_properties?
      - I suppose right now we just throw a SerializationError.  If you'd rather emit the signal 'serialize_unknown_property' and add it to the table, that seems reasonable, so people can programmatically handle it without the program failing.  I might like to call g_error, though, as in theory people shouldn't be serializing objects whose properties will fail. :D
   
 
 
> Cheerio,
>   Richard Schwarting
>
>
> On Tue, Aug 6, 2013 at 5:31 PM, Richard Schwarting <aquarichy gmail com> wrote:
>>
>> I'll probably have to get back to you with tomorrow on comments on your branch.  Sorry.
>>
>>
>> On Tue, Aug 6, 2013 at 4:58 PM, Richard Schwarting <aquarichy gmail com> wrote:
>>>
>>> I finally pushed the change to serialization to gsoc2013 that fixes it.  The main change (breaking API) is that instead of gxml_serialization_serialize_object () returning a GXmlNode (which was the document_element of a document) it now returns a GXmlDocument (and its document_element is what you used to get).  Consequently, gxml_serialization_deserialize_object () now takes a GXmlDocument, which incidentally cleans up the API usage slightly (you don't have to read a document and then pass doc.document_element; instead just pass the doc!). 
>>>
>>> I've requested a mailing list:
>>> https://bugzilla.gnome.org/show_bug.cgi?id=705571
>>>
>>> Let me know about the invalid UTF-8 strings.  I think they were related to the case where the memory was already freed before the serialize_object () method returned the GXmlNode.
>>>
>>> I'll look into parallel installation.  My current plan for releasing is:
>>> 0.3.2: all the non-API changes pushed here, so people who are using 0.3.x can keep doing it with the fixes.
>>> 0.4: most of the API breaking things, of which there are a few more coming (all classes are going to reflect their corresponding names from the W3 DOM spec; it's silly that I let them vary to begin with).
>>>
>>> Regarding IRC, right now I lurk in #gtk+ and #vala, so if you just want to talk to me, you can find me there.  We can create an impromptu #gxml one for now if you like, or we can formally do it.
>>>
>>> I'm going to review and comment on your serialization finally.
>>>
>>> Sorry for the delay, GUADEC is busy, and so is my Masters, so I've had a hard time being as responsive as I should be.
>>>
>>> Cheerio, 
>>>   Richard Schwarting
>>>
>>>
>>>
>>> On Mon, Aug 5, 2013 at 4:14 AM, Daniel Espinosa <esodan gmail com> wrote:
>>>>
>>>> Great!
>>>>
>>>> I'm not too much on irc. But I could if you consider benefit.
>>>>
>>>> What is the channel we can join?
>>>>
>>>> Really thanks for your advise. For now I think  gxml serializable is almost ready, but really considering to use TextReader/Writer for serialization because memory use, or add an interface to implement it, I've created a branch locally for this witch I can share i you want, but at the end, are you planning to release shortly a 0.4, if so my plans on TextReader/Writer must wait.
>>>>
>>>> I'm planning to provide gxml parallel features with C# Serializer, I have a project on sourceforge called OpenSCLConfigurator using that tech but I want to rewrite it using Vala.
>>>>
>>>> El ago 4, 2013 8:45 p.m., "Richard Schwarting" <aquarichy gmail com> escribió:
>>>>
>>>>> Just to keep you up to date, hacking resumes tomorrow (the last day of talks at GUADEC ended today).  My primary goals are this, reviewing Adam Ples' patch in Bugzilla, and preparing releases. 
>>>>>
>>>>> Are you on IRC much?
>>>>>
>>>>>
>>>>> On Wed, Jul 31, 2013 at 2:16 PM, Daniel Espinosa <esodan gmail com> wrote:
>>>>>>
>>>>>> Great! Thanks.
>>>>>>
>>>>>> I'm waiting for it in order to merge again and add my own test cases to serializable and fix Serialization to handle two ways: the actual one in 0.3 and the new one in up coming 0.4
>>>>>>
>>>>>> By the way, I think we need to add 0.4 sufix to .pc, Gir, subdirectories and others, in GXml in order to  allow parallel installation with 0.3 series.
>>>>>>
>>>>>> One example is Gda.
>>>>>>
>>>>>> https://git.gnome.org/browse/libgda/tree/libgda/Makefile.am
>>>>>>
>>>>>> As far I can see you have all infrastructure to make the change.
>>>>>>
>>>>>> An for last, are there a gxml mailing list? To make this discussion public?
>>>>>>
>>>>>> El jul 31, 2013 12:29 a.m., "Richard Schwarting" <aquarichy gmail com> escribió:
>>>>>>
>>>>>>> Oh, I can fix that.  I'll do that once I arrive at GUADEC.  Thanks for the heads up.
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Jul 30, 2013 at 6:59 PM, Daniel Espinosa <esodan gmail com> wrote:
>>>>>>>>
>>>>>>>> Just to know if I need to file a bug.
>>>>>>>>
>>>>>>>> gsoc2013 branch, witch I've synced with my serialize branch, fails to run make check command.
>>>>>>>>
>>>>>>>> My problem is at  /gxml/serialization/xml_serialize
>>>>>>>>
>>>>>>>> fruit_xml.to_string () returns invalid UTF8 strings, but it is not consistent, some rare cases just get in an infinity loop, others messages like;
>>>>>>>>
>>>>>>>> output error : string is not in UTF-8
>>>>>>>>
>>>>>>>> Some times empty strings.
>>>>>>>>
>>>>>>>> Are your make check command works for you?
>>>>>>>>
>>>>>>>> --
>>>>>>>> Trabajar, la mejor arma para tu superación
>>>>>>>> "de grano en grano, se hace la arena" (R) (en trámite, pero para los cuates: LIBRE)
>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]