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.

> === 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.

-- 
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs