Nir Soffer has posted comments on this change.

Change subject: yml: generate api doc from the schema
......................................................................


Patch Set 24:

(4 comments)

https://gerrit.ovirt.org/#/c/56387/24/lib/api/Makefile.am
File lib/api/Makefile.am:

Line 49: 
Line 50: vdsm-api.html: schema_to_html.py vdsm-api.yml
Line 51:        @echo "  Generate $@"
Line 52:        chmod u+w $(srcdir)
Line 53:        PYTHONDONTWRITEBYTECODE=1 
PYTHONPATH=$(srcdir)/../:$(srcdir)/../vdsm \
> This is the same command which was used before. I removed it some time ago 
Since the code was removed, there is no reason to restore it. Lets add the 
simplest and cleanest code we can.
Line 54:            $(PYTHON) $(srcdir)/schema_to_html.py \


Line 51:        @echo "  Generate $@"
Line 52:        chmod u+w $(srcdir)
Line 53:        PYTHONDONTWRITEBYTECODE=1 
PYTHONPATH=$(srcdir)/../:$(srcdir)/../vdsm \
Line 54:            $(PYTHON) $(srcdir)/schema_to_html.py \
Line 55:                      $(srcdir)/vdsm-api.yml $(srcdir)/$@
> As above
Same


https://gerrit.ovirt.org/#/c/56387/24/lib/api/vdsmapi.py
File lib/api/vdsmapi.py:

Line 156:             return self._methods[rep.id]
Line 157:         except KeyError:
Line 158:             raise MethodNotFound(rep.id)
Line 159: 
Line 160:     def get_all_methods(self):
> It should
Looking at the other apis, I'm not sure it is better, this is very consistent 
with the other get_xxx functions.

Bug there is a bigger problem, you return a mutable value, and the caller can 
modify the schema by mistake. Better return a copy of the schema. Since the 
schema is a deeply nested, we probably want a deep copy. The fastest way to do 
this is using utils.pickelcopy.
Line 161:         return self._methods
Line 162: 
Line 163:     def get_type(self, type_name):
Line 164:         try:


Line 165:             return self._types[type_name]
Line 166:         except KeyError:
Line 167:             raise TypeNotFound(type_name)
Line 168: 
Line 169:     def get_all_types(self):
> It should
Same as above, need to copy the _types dict.
Line 170:         return self._types
Line 171: 
Line 172:     def _check_primitive_type(self, t, value, name):
Line 173:         condition = PRIMITIVE_TYPES.get(t)


-- 
To view, visit https://gerrit.ovirt.org/56387
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e0cdd7322b06899a8fb895a5bfee4d2e0e3bc8c
Gerrit-PatchSet: 24
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: gerrit-hooks <[email protected]>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]

Reply via email to