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-06:

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.

     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?

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

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 :)

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

@@ -91,3 +132,18 @@
                 valid = False
                 break
         return valid, lines
+
+ def dict_to_xml(self):
+ """
+ Converts the necessary values into an XML tree.
+
+ :return The XML tree as a string, or an empty string if the inserted
+ values are not valid.
+ """
+ xml_string = ""
+ valid, lines = self.valid_values(self.values)
+ if valid:
+ values = self.values_to_dict(valid, lines)
+ values.update(self._all_values_to_dict())
+ xml_string = DictToXml(values).dict_to_tree()
+ return xml_string

These methods here don't have much sense to me, and that's the main reason I'm opposing the dict. It's very confusing to look at :/ But let's not do anything for now.

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

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

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?

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

=== 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 :)

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

@@ -91,3 +132,18 @@
                 valid = False
                 break
         return valid, lines
+
+    def dict_to_xml(self):
+        """
+        Converts the necessary values into an XML tree.
+
+        :return The XML tree as a string, or an empty string if the inserted
+        values are not valid.
+        """
+        xml_string = ""
+        valid, lines = self.valid_values(self.values)
+        if valid:
+            values = self.values_to_dict(valid, lines)
+            values.update(self._all_values_to_dict())
+            xml_string = DictToXml(values).dict_to_tree()
+        return xml_string

These methods here don't have much sense to me, and that's the main reason I'm opposing the dict. It's very confusing to look at :/ But let's not do anything for now.

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

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

review: Needs Fixing

« Back to merge proposal