Zhou Zheng Sheng has posted comments on this change. Change subject: add a new tool for auto-generate python entities from schema ......................................................................
Patch Set 5: (7 inline comments) Partially reviewed. I will go on reviewing the patch. .................................................... Commit Message Line 21: I thought we could use the diff and patch way. I mean firstly we can edit the file manually, secondly diff the modified and the original version, then we get a patch, at last we can reuse the patch to avoid manually editing. The diff and patch way may fail when the original file changes, and the patch may not apply cleanly. Maybe the genparams.py tool can survive in this case. In my opinion this is the main advantage of writing a new tool. .................................................... File src/codegen/paramsconf.py Line 1: ######################!/usr/bin/python Do not have to add "#!/usr/bin/python" in this file, since this file will only be imported by genparams.py. Line 4: # this maybe useful, for some editor such as vi or eclips "eclips" or "Eclipse"? Line 18: GENERATEDSSUPER_ATTRS = """ I think GENERATEDSSUPER_ATTRS is a module private variable, so I suggest prefix an underscore to its name. This will gives the user more clues on how to write this config file. It seems that addfunctionlist, globalfunctionlist and substitutefunctionlist are variables for genparams.py to use, and their names are important. Other variables like GENERATEDSSUPER_ATTRS will only be used inside this file, referring by those important dictionaries, so the names of these variables can be arbitrary. If I were the user, I would be happy to see unimportant variables are prefixed with underscore, so that I would not have to think which variables are important. Line 80: "GeneratedsSuper": GENERATEDSSUPER_ATTRS} Since variables like addfunctionlist are actually dictionaries. Maybe a name like "addfunctions" is less misleading. Line 136: # the function dict which will be take the same function in the params.py I suggest "functions in the following dict will replace the functions with the same names in params.py". Line 140: # second,which class the functon belong to So empty class name means the target function is a global function? -- To view, visit http://gerrit.ovirt.org/4880 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I916f63f1ac68ce2e31456a90c1291997f671af99 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine-sdk Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng <shao...@linux.vnet.ibm.com> Gerrit-Reviewer: Adam Litke <a...@us.ibm.com> Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com> Gerrit-Reviewer: ShaoHe Feng <shao...@linux.vnet.ibm.com> Gerrit-Reviewer: Shu Ming <shum...@linux.vnet.ibm.com> Gerrit-Reviewer: Zhou Zheng Sheng <zhshz...@linux.vnet.ibm.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
