linaro-ci-dashboard

Code review comment for lp:~milo/linaro-ci-dashboard/xml-to-dict

xml-to-dict
Merge into trunk

Revision history for this message

Stevan Radaković (stevanr) wrote on 2012-09-07:

On 09/07/2012 10:37 AM, Milo Casagrande wrote:
> Hello Stevan,
>
> thanks for the review!
>
> On Thu, Sep 6, 2012 at 3:55 PM, Stevan Radaković
> <email address hidden> wrote:
>> Review: Needs Fixing
>>
>> Hey Milo great work !!1
>> Before discussing the dict necessity with danilo, I'll do a review here and lets land this, with tendency to refactor things later.
>>
>>
>>
>> === modified file 'dashboard/frontend/android_textfield_loop/tests/test_android_textfield_loop_model.py'
>> --- dashboard/frontend/android_textfield_loop/tests/test_android_textfield_loop_model.py 2012-09-05 13:59:57 +0000
>> +++ dashboard/frontend/android_textfield_loop/tests/test_android_textfield_loop_model.py 2012-09-06 11:09:25 +0000
>> @@ -24,9 +24,9 @@
>>
>> A_NAME = 'a-build'
>> B_NAME = 'b-build'
>> - VALID_VALUES = 'a=2\nb=3'
>> + VALID_VALUES = u'a=2\nb=3'
>> VALID_LINES = ['a=2', 'b=3']
>> - NON_VALID_VALUES = 'a:2\nb=3'
>> + NON_VALID_VALUES = u'a:2\nb=3'
>> NON_VALID_LINES = ['a:2', 'b=3']
>> VALID_DICT = {'a': '2', 'b': '3'}
>>
>> @@ -52,6 +52,7 @@
>> def test_valid_values_wrong(self):
>> self.assertEqual((False, self.NON_VALID_LINES),
>> AndroidTextFieldLoop.valid_values(
>> +<<<<<<< TREE
>> self.NON_VALID_VALUES))
>>
>> def test_schedule_build(self):
>> @@ -62,3 +63,27 @@
>> def test_schedule_build_invalid(self):
>> build = self.non_valid_android_loop.schedule_build()
>> self.assertEqual(build.result_xml, {})
>> +=======
>> + self.NON_VALID_VALUES))
>> +
>> + def test_schedule_build(self):
>> + build = self.android_loop.schedule_build()
>> + expected_out = '<?xml version="1.0" encoding="UTF-8"?>\n<!DOCTYPE ' \
>> + 'loop [\n <!ELEMENT loop (description?,fields)>\n' \
>> + ' <!ELEMENT description (#PCDATA)>\n ' \
>> + '<!ELEMENT fields (field+)>\n <!ELEMENT field ' \
>> + '(#PCDATA)>\n <!ATTLIST field name CDATA ' \
>> + '#REQUIRED>\n <!ATTLIST field type (text|int|bool) ' \
>> + '"text">\n]><loop><fields><field name="a">2</field>' \
>> + '<field name="b">3</field><field name="name">a-build' \
>> + '</field><field name="is_restricted">False</field>' \
>> + '<field name="is_official">False</field>' \
>> + '<field name="type">AndroidTextFieldLoop</field>' \
>> + '</fields></loop>'
>> + self.assertEqual(expected_out, build.result_xml)
>> + self.assertEqual(build.status, "success")
>> +
>> + def test_schedule_build_invalid(self):
>> + build = self.non_valid_android_loop.schedule_build()
>> + self.assertEqual("", build.result_xml)
>> +>>>>>>> MERGE-SOURCE
>>
>>
>> What's with all the white spaces in the expected out? Can't we do strip() somewhere in the code and output xml without new lines and white spaces?
> The spaces are just in the DTD, that is a copy paste of the DTD as
> defined in the HACKING file. That's the only part with spaces and
> newlines. The XML output from the "parser" is just a one-liner.
>
>> === modified file 'dashboard/frontend/models/loop_build.py'
>> --- dashboard/frontend/models/loop_build.py 2012-09-05 13:23:05 +0000
>> +++ dashboard/frontend/models/loop_build.py 2012-09-06 11:09:25 +0000
>> @@ -18,6 +18,7 @@
>>
>> from django.db import models
>> from frontend.models.loop import Loop
>> +from dashboard.lib.xml_to_dict import XmlToDict
>>
>>
>> class LoopBuild(models.Model):
>> @@ -47,3 +48,15 @@
>> else:
>> self.build_number = 1
>> super(LoopBuild, self).save(*args, **kwargs)
>> +
>> + def get_build_result(self):
>> + """
>> + Returns a Python dictionary representation of the build XML stored.
>> +
>> + :return A dictionary of the XML result.
>> + """
>> + dict_result = {}
>> + if self.result_xml:
>> + xml_to_dict = XmlToDict(self.result_xml)
>> + dict_result = xml_to_dict.tree_to_dict()
>> + return dict_result
>>
>>
>> I think this should be where our remote asynchronous call should go, but I'll refactor that as part of my next WI. Just let me know if you have other opinion (this code will be moved as overridden method in textfield_loop).
> Makes sense to me, as we discussed a little bit yesterday.
>
>>
>> === modified file 'dashboard/frontend/models/textfield_loop.py'
>> --- dashboard/frontend/models/textfield_loop.py 2012-09-05 13:59:57 +0000
>> +++ dashboard/frontend/models/textfield_loop.py 2012-09-06 11:09:25 +0000
>> ................
>> @@ -55,14 +57,40 @@
>> return build
>>
>> def values_to_dict(self):
>> +=======
>> + from frontend.models.loop_build import LoopBuild
>> + build = LoopBuild()
>> + build.loop = self
>> + build.duration = 0.00
>> +
>> + try:
>> + build.result_xml = self.dict_to_xml()
>>
>>
>> dict_to_xml is not the right way to call this method here. What we are doing is 'values_to_xml' or 'values_to_xml_through_dict', the second one being an overkill :)
> Used "values_to_xml".
>
>> @@ -71,6 +99,19 @@
>>
>> return text_to_dict
>>
>> + def _all_values_to_dict(self):
>> + """
>> + Returns a dict representation of the necessary fields for the loops.
>> +
>> + :return A Python dictionary with the necessary fields.
>> + """
>> + # TODO need a better way to serialize the model
>> + all_values_dict = {'is_official': self.is_official,
>> + 'is_restricted': self.is_restricted,
>> + 'name': self.name,
>> + 'type': self.type}
>> + return all_values_dict
>> +
>>
>> Have you considered usgin "model_to_dict" from "django.forms.models" ?
>> This can also maybe help you with some other stuff, youll get the idea ..
> Yes, but I will have to move it up into Loop, since TextFieldLoop is
> an abstract class, and that method was not working (or I was not able
> to make it work) with it.
> Now that I think I will move it into Loop, but I will need to filter
> on the fields, otherwise I will get back also fields that we do not
> need, included the "values" one that we already parse and split.

Good idea... We could also check if we can somehow make use of the
'exclude' option in the Meta subclass for filtering out the fields; or
create our own option if it's not good match for us.

>
>> === added file 'dashboard/lib/xml_fields.py'
>> --- dashboard/lib/xml_fields.py 1970-01-01 00:00:00 +0000
>> +++ dashboard/lib/xml_fields.py 2012-09-06 11:09:25 +0000
>> ..........
>>
>> I don't like this file :/
>> Can't we define XSD file and do the XML validation with it? Something like http://lxml.de/validation.html#xmlschema ?
>> Not sure if it's supported with our library but it's worth checking.
>> After validating, we just pull the nodes and attrs we need, cause we know they are present...
> Nope, that part is not supported, xml.etree.ElementTree does not
> support any validation at all. That is based on lxml, we can use that
> if necessary, but I do not see it really necessary...
> The problem is that we need the name of the elements to create the
> XML, that file is mostly used for that, not for reading or validating
> the tree. Since we create the tree, if we do not make it valid, then
> there is a problem. The only thing used when reading, is checking if
> the element is a "field" element, and treat it accordingly (done
> because we have a description element, that if used we will need to
> treat it differently since that will be a CDATA block, not a PCDATA
> one).
> Also, we have the DTD, so we do not need XSD definition, and lxml
> supports validation with mostly everything (DTD included) if we want
> to use that.
>
>> === added file 'dashboard/lib/xml_to_dict.py'
>> --- dashboard/lib/xml_to_dict.py 1970-01-01 00:00:00 +0000
>> +++ dashboard/lib/xml_to_dict.py 2012-09-06 11:09:25 +0000
>> .........
>> +class XmlToDict(object):
>> + """
>> + Class to convert an XML string into a Python dictionary.
>> + This applies to the XML as defined for the loop chaining.
>> +
>> + Simple usage:
>> + xml_2_dict = XmlToDict(xml_string).tree_to_dict()
>> + """
>> +
>> + def __init__(self, xml):
>> + """
>> + Initialize the XmlToDict class.
>> +
>> + :param xml: The XML tree as a string.
>> + :type xml str
>> + """
>> + assert isinstance(xml, str)
>>
>> Assert? Let's go with exceptions please. We can create a custom one for our class. There are more asserts below in the code so I will not comment them all.
> Going to fix this.
> Ciao.
>

On 09/07/2012 10:37 AM, Milo Casagrande wrote:
> Hello Stevan,
>
> thanks for the review!
>
> On Thu, Sep 6, 2012 at 3:55 PM, Stevan Radaković
> <stevan.radakovic@linaro.org> wrote:
>> Review: Needs Fixing
>>
>> Hey Milo great work !!1
>> Before discussing the dict necessity with danilo, I'll do a review here and lets land this, with tendency to refactor things later.
>>
>>
>>
>> === modified file 'dashboard/frontend/android_textfield_loop/tests/test_android_textfield_loop_model.py'
>> --- dashboard/frontend/android_textfield_loop/tests/test_android_textfield_loop_model.py        2012-09-05 13:59:57 +0000
>> +++ dashboard/frontend/android_textfield_loop/tests/test_android_textfield_loop_model.py        2012-09-06 11:09:25 +0000
>> @@ -24,9 +24,9 @@
>>
>>      A_NAME = 'a-build'
>>      B_NAME = 'b-build'
>> -    VALID_VALUES = 'a=2\nb=3'
>> +    VALID_VALUES = u'a=2\nb=3'
>>      VALID_LINES = ['a=2', 'b=3']
>> -    NON_VALID_VALUES = 'a:2\nb=3'
>> +    NON_VALID_VALUES = u'a:2\nb=3'
>>      NON_VALID_LINES = ['a:2', 'b=3']
>>      VALID_DICT = {'a': '2', 'b': '3'}
>>
>> @@ -52,6 +52,7 @@
>>      def test_valid_values_wrong(self):
>>          self.assertEqual((False, self.NON_VALID_LINES),
>>                           AndroidTextFieldLoop.valid_values(
>> +<<<<<<< TREE
>>                               self.NON_VALID_VALUES))
>>
>>      def test_schedule_build(self):
>> @@ -62,3 +63,27 @@
>>      def test_schedule_build_invalid(self):
>>          build = self.non_valid_android_loop.schedule_build()
>>          self.assertEqual(build.result_xml, {})
>> +=======
>> +                             self.NON_VALID_VALUES))
>> +
>> +    def test_schedule_build(self):
>> +        build = self.android_loop.schedule_build()
>> +        expected_out = '<?xml version="1.0" encoding="UTF-8"?>\n<!DOCTYPE ' \
>> +                       'loop [\n    <!ELEMENT loop (description?,fields)>\n' \
>> +                       '    <!ELEMENT description (#PCDATA)>\n    ' \
>> +                       '<!ELEMENT fields (field+)>\n    <!ELEMENT field ' \
>> +                       '(#PCDATA)>\n    <!ATTLIST field name CDATA ' \
>> +                       '#REQUIRED>\n    <!ATTLIST field type (text|int|bool) ' \
>> +                       '"text">\n]><loop><fields><field name="a">2</field>' \
>> +                       '<field name="b">3</field><field name="name">a-build' \
>> +                       '</field><field name="is_restricted">False</field>' \
>> +                       '<field name="is_official">False</field>' \
>> +                       '<field name="type">AndroidTextFieldLoop</field>' \
>> +                       '</fields></loop>'
>> +        self.assertEqual(expected_out, build.result_xml)
>> +        self.assertEqual(build.status, "success")
>> +
>> +    def test_schedule_build_invalid(self):
>> +        build = self.non_valid_android_loop.schedule_build()
>> +        self.assertEqual("", build.result_xml)
>> +>>>>>>> MERGE-SOURCE
>>
>>
>> What's with all the white spaces in the expected out? Can't we do strip() somewhere in the code and output xml without new lines and white spaces?
> The spaces are just in the DTD, that is a copy paste of the DTD as
> defined in the HACKING file. That's the only part with spaces and
> newlines. The XML output from the "parser" is just a one-liner.
>
>> === modified file 'dashboard/frontend/models/loop_build.py'
>> --- dashboard/frontend/models/loop_build.py     2012-09-05 13:23:05 +0000
>> +++ dashboard/frontend/models/loop_build.py     2012-09-06 11:09:25 +0000
>> @@ -18,6 +18,7 @@
>>
>>  from django.db import models
>>  from frontend.models.loop import Loop
>> +from dashboard.lib.xml_to_dict import XmlToDict
>>
>>
>>  class LoopBuild(models.Model):
>> @@ -47,3 +48,15 @@
>>              else:
>>                  self.build_number = 1
>>          super(LoopBuild, self).save(*args, **kwargs)
>> +
>> +    def get_build_result(self):
>> +        """
>> +        Returns a Python dictionary representation of the build XML stored.
>> +
>> +        :return A dictionary of the XML result.
>> +        """
>> +        dict_result = {}
>> +        if self.result_xml:
>> +            xml_to_dict = XmlToDict(self.result_xml)
>> +            dict_result = xml_to_dict.tree_to_dict()
>> +        return dict_result
>>
>>
>> I think this should be where our remote asynchronous call should go, but I'll refactor that as part of my next WI. Just let me know if you have other opinion (this code will be moved as overridden method in textfield_loop).
> Makes sense to me, as we discussed a little bit yesterday.
>
>>
>> === modified file 'dashboard/frontend/models/textfield_loop.py'
>> --- dashboard/frontend/models/textfield_loop.py 2012-09-05 13:59:57 +0000
>> +++ dashboard/frontend/models/textfield_loop.py 2012-09-06 11:09:25 +0000
>> ................
>> @@ -55,14 +57,40 @@
>>          return build
>>
>>      def values_to_dict(self):
>> +=======
>> +        from frontend.models.loop_build import LoopBuild
>> +        build = LoopBuild()
>> +        build.loop = self
>> +        build.duration = 0.00
>> +
>> +        try:
>> +            build.result_xml = self.dict_to_xml()
>>
>>
>> dict_to_xml is not the right way to call this method here. What we are doing is 'values_to_xml' or 'values_to_xml_through_dict', the second one being an overkill :)
> Used "values_to_xml".
>
>> @@ -71,6 +99,19 @@
>>
>>          return text_to_dict
>>
>> +    def _all_values_to_dict(self):
>> +        """
>> +        Returns a dict representation of the necessary fields for the loops.
>> +
>> +        :return A Python dictionary with the necessary fields.
>> +        """
>> +        # TODO need a better way to serialize the model
>> +        all_values_dict = {'is_official': self.is_official,
>> +                           'is_restricted': self.is_restricted,
>> +                           'name': self.name,
>> +                           'type': self.type}
>> +        return all_values_dict
>> +
>>
>> Have you considered usgin "model_to_dict" from "django.forms.models" ?
>> This can also maybe help you with some other stuff, youll get the idea ..
> Yes, but I will have to move it up into Loop, since TextFieldLoop is
> an abstract class, and that method was not working (or I was not able
> to make it work) with it.
> Now that I think I will move it into Loop, but I will need to filter
> on the fields, otherwise I will get back also fields that we do not
> need, included the "values" one that we already parse and split.

Good idea... We could also check if we can somehow make use of the
'exclude' option in the Meta subclass for filtering out the fields; or
create our own option if it's not good match for us.

>
>> === added file 'dashboard/lib/xml_fields.py'
>> --- dashboard/lib/xml_fields.py 1970-01-01 00:00:00 +0000
>> +++ dashboard/lib/xml_fields.py 2012-09-06 11:09:25 +0000
>> ..........
>>
>> I don't like this file :/
>> Can't we define XSD file and do the XML validation with it? Something like http://lxml.de/validation.html#xmlschema ?
>> Not sure if it's supported with our library but it's worth checking.
>> After validating, we just pull the nodes and attrs we need, cause we know they are present...
> Nope, that part is not supported, xml.etree.ElementTree does not
> support any validation at all. That is based on lxml, we can use that
> if necessary, but I do not see it really necessary...
> The problem is that we need the name of the elements to create the
> XML, that file is mostly used for that, not for reading or validating
> the tree. Since we create the tree, if we do not make it valid, then
> there is a problem. The only thing used when reading, is checking if
> the element is a "field" element, and treat it accordingly (done
> because we have a description element, that if used we will need to
> treat it differently since that will be a CDATA block, not a PCDATA
> one).
> Also, we have the DTD, so we do not need XSD definition, and lxml
> supports validation with mostly everything (DTD included) if we want
> to use that.
>
>> === added file 'dashboard/lib/xml_to_dict.py'
>> --- dashboard/lib/xml_to_dict.py        1970-01-01 00:00:00 +0000
>> +++ dashboard/lib/xml_to_dict.py        2012-09-06 11:09:25 +0000
>> .........
>> +class XmlToDict(object):
>> +    """
>> +    Class to convert an XML string into a Python dictionary.
>> +    This applies to the XML as defined for the loop chaining.
>> +
>> +    Simple usage:
>> +        xml_2_dict = XmlToDict(xml_string).tree_to_dict()
>> +    """
>> +
>> +    def __init__(self, xml):
>> +        """
>> +        Initialize the XmlToDict class.
>> +
>> +        :param xml: The XML tree as a string.
>> +        :type xml str
>> +        """
>> +        assert isinstance(xml, str)
>>
>> Assert? Let's go with exceptions please. We can create a custom one for our class. There are more asserts below in the code so I will not comment them all.
> Going to fix this.
> Ciao.
>

« Back to merge proposal