Change in vdsm[master]: check the syntaxs of optional parameters in json schema

ShaoHe Feng has uploaded a new change for review.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

check the syntaxs of optional parameters in json schema

There are so many syntaxs error of optional parameters define in
json schema
This patch can both check the descrption errors of optional parameters
missing #optional and the define error of optional parameters missing *

Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Signed-off-by: ShaoHe Feng 
---
M vdsm_api/process-schema.py
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/46/10446/1

diff --git a/vdsm_api/process-schema.py b/vdsm_api/process-schema.py
index 8244995..5db6966 100755
--- a/vdsm_api/process-schema.py
+++ b/vdsm_api/process-schema.py
@@ -22,6 +22,11 @@
 import sys
 import re
 import vdsmapi
+try:
+    from collections import OrderedDict
+    OrderedDict  # make pyflakes happy
+except ImportError:
+    from ordereddict import OrderedDict

html_escape_table = {
     "&": "&",
@@ -143,6 +148,18 @@
                 symbol[mode][name] = desc
                 # Track the name in case there is are multiple lines to append
                 last_arg = name
+                # check optional parameters
+                if desc.startswith('#optional '):
+                    assert ('*' + name in symbol['data'].keys()), \
+                        ('Define of %s optional parameter error:\n\t%s should '
+                         'start with *' % (symbol['name'], name))
+                if isinstance(symbol['data'], OrderedDict):
+                    if '*' + name in symbol['data'].keys():
+                        assert desc.startswith('#optional '), \
+                            ('Description of %s optional parameter error:\n\t'
+                             'The description of optional parameter "%s" '
+                             'should start with "#optional"' %
+                             (symbol['name'], name))
             else:
                 # Just append it to the last one we added
                 symbol[mode][last_arg] += ' ' + line

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/527/ (2/2)

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/492/ (1/2)

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 1: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/492/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/527/ : FAILURE

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Zhou Zheng Sheng has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 1: I would prefer that you didn't submit this

Your patch is good. However there are some errors in the json schema file and your patch exposes these errors, that's why Jenkins unit test failed to build. You can have a look at the last part of the report http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/527/console .

I think you can use the technique proposed in this patch to find all the #optional missing errors and fix them in the json schema, then rebase this patch on these fixes.

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

ShaoHe Feng has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 1:

yes. jenkins report errors means my patch is right.

there are so many optional parameters errors in json schema.

I should make sure that they are optional with the author who define these optional parameters .

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

ShaoHe Feng has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 1: Verified

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/495/ (1/2)

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/530/ (2/2)

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/495/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/530/ : SUCCESS

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

ShaoHe Feng has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2: Verified

--
To view, visit http://gerrit.ovirt.org/10446
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Saggi Mizrahi has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2: Code-Review-1

this patch is

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2:

this patch is very old, and surely requires a rebase.

Itamar Heim has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2:

ping

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: check the syntaxs of optional parameters in json schema
......................................................................

Patch Set 2:

(5 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2012-12-28 00:27:20 +0800
Line 4: Commit:     ShaoHe Feng 
Line 5: CommitDate: 2012-12-28 14:30:30 +0800
Line 6: 
Line 7: check the syntaxs of optional parameters in json schema
Detect optional parameter syntax errors in JSON schema
Line 8: 
Line 9: There are so many syntaxs error of optional parameters define in
Line 10: json schema
Line 11: This patch can both check the descrption errors of optional parameters

Line 8: 
Line 9: There are so many syntaxs error of optional parameters define in
Line 10: json schema
Line 11: This patch can both check the descrption errors of optional parameters
Line 12: missing #optional and the define error of optional parameters missing *
There are too many syntax errors using optional parameters in JSON schema. This patch detects these errors.
Line 13: 
Line 14: Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235

....................................................
File vdsm_api/process-schema.py
Line 23: import re
Line 24: import vdsmapi
Line 25: try:
Line 26:     from collections import OrderedDict
Line 27:     OrderedDict  # make pyflakes happy
Can we fix pyflakes instead? This type of code make me sad :-)
Line 28: except ImportError:
Line 29:     from ordereddict import OrderedDict
Line 30: 
Line 31: html_escape_table = {

Line 151:                 # check optional parameters
Line 152:                 if desc.startswith('#optional'):
Line 153:                     assert ('*' + name in symbol['data'].keys()), \
Line 154:                         ('Define of %s optional parameter error:\n\t%s should '
Line 155:                          'start with *' % (symbol['name'], name))
Since we raise ValueError bellow - why not raise it here also?
Line 156:                 if isinstance(symbol['data'], OrderedDict):
Line 157:                     if '*' + name in symbol['data'].keys():
Line 158:                         assert desc.startswith('#optional'), \
Line 159:                             ('Description of %s optional parameter error:\n\t'

Line 158:                         assert desc.startswith('#optional'), \
Line 159:                             ('Description of %s optional parameter error:\n\t'
Line 160:                              'The description of optional parameter "%s" '
Line 161:                              'should start with "#optional"' %
Line 162:                              (symbol['name'], name))
ValueError?
Line 163:             else:
Line 164:                 # Just append it to the last one we added
Line 165:                 symbol[mode][last_arg] += ' ' + line
Line 166:         elif mode == 'info_return':

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4747/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5547/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5627/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

ShaoHe Feng has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 2:

(5 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2012-12-28 00:27:20 +0800
Line 4: Commit:     ShaoHe Feng 
Line 5: CommitDate: 2012-12-28 14:30:30 +0800
Line 6: 
Line 7: check the syntaxs of optional parameters in json schema
Done
Line 8: 
Line 9: There are so many syntaxs error of optional parameters define in
Line 10: json schema
Line 11: This patch can both check the descrption errors of optional parameters

Line 8: 
Line 9: There are so many syntaxs error of optional parameters define in
Line 10: json schema
Line 11: This patch can both check the descrption errors of optional parameters
Line 12: missing #optional and the define error of optional parameters missing *
Done
Line 13: 
Line 14: Change-Id: I94f92459ee8787780a54a510b6f8cc074fb2a235

....................................................
File vdsm_api/process-schema.py
Line 23: import re
Line 24: import vdsmapi
Line 25: try:
Line 26:     from collections import OrderedDict
Line 27:     OrderedDict  # make pyflakes happy
A good idea. check other vdsm code, they do the same way.

If we can fix pyflakes, it is more better.
Line 28: except ImportError:
Line 29:     from ordereddict import OrderedDict
Line 30: 
Line 31: html_escape_table = {

Line 151:                 # check optional parameters
Line 152:                 if desc.startswith('#optional'):
Line 153:                     assert ('*' + name in symbol['data'].keys()), \
Line 154:                         ('Define of %s optional parameter error:\n\t%s should '
Line 155:                          'start with *' % (symbol['name'], name))
Done
Line 156:                 if isinstance(symbol['data'], OrderedDict):
Line 157:                     if '*' + name in symbol['data'].keys():
Line 158:                         assert desc.startswith('#optional'), \
Line 159:                             ('Description of %s optional parameter error:\n\t'

Line 158:                         assert desc.startswith('#optional'), \
Line 159:                             ('Description of %s optional parameter error:\n\t'
Line 160:                              'The description of optional parameter "%s" '
Line 161:                              'should start with "#optional"' %
Line 162:                              (symbol['name'], name))
Done
Line 163:             else:
Line 164:                 # Just append it to the last one we added
Line 165:                 symbol[mode][last_arg] += ' ' + line
Line 166:         elif mode == 'info_return':

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3: Verified+1

Looks good, but can we have few tests that show that code without error does not raise, and code with relevant errors raises?

See tests/schemaTests.py

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3: -Verified Code-Review+1

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

Itamar Heim has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3:

ping

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

Adam Litke has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

This just requires a minor change to the wording of the error messages.  Also, when it is rebased, the current schema should be checked for errors and this patch should include the fixes to bring the schema into compliance.

....................................................
File vdsm_api/process-schema.py
Line 165:                         raise ValueError(
Line 166:                             'Description of %s optional parameter error:\n'
Line 167:                             '\tThe description of optional parameter "%s" '
Line 168:                             'should start with "#optional"' %
Line 169:                             (symbol['name'], name))
The error messages above always assume that if the optional designation appears only in the documentation comment or the 'data' structure field (but not both) that the fix is to add the missing designation.  To properly fix these errors we must be careful to check whether the parameter is really optional or not.  Therefore, for either case above I would recommend a more general error message:

Symbol '%s' optional parameter declaration error.  The description of parameter '%s' is tagged '#optional' but not defined as optional.

Symbol '%s' optional parameter declaration error.  The parameter '%s' is defined as optional but the description is missing the '#optional' tag.
Line 170:             else:
Line 171:                 # Just append it to the last one we added
Line 172:                 symbol[mode][last_arg] += ' ' + line
Line 173:         elif mode == 'info_return':

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3: -Code-Review

(1 comment)

....................................................
File vdsm_api/process-schema.py
Line 165:                         raise ValueError(
Line 166:                             'Description of %s optional parameter error:\n'
Line 167:                             '\tThe description of optional parameter "%s" '
Line 168:                             'should start with "#optional"' %
Line 169:                             (symbol['name'], name))
The real issue here is the need for both declaration and definition of the optional property. It does not make sense that we need mark a property twice.

We should change the schema format so the optional parameter property is defined only once, and if it is defined, the system will add the optional note to the description in post processing step.

For example:

##
    # @Foo:
    #
    # Description...
    #
    # @bar:          description of bar
    #
    ##
    {'type': 'Foo',                                       
      'data': {'*bar': 'str'}}

After post processing (e.g. html):

<dt>bar</dt>
    <dd>description of bar (optional)</dd>

This format can not such inconsistency.
Line 170:             else:
Line 171:                 # Just append it to the last one we added
Line 172:                 symbol[mode][last_arg] += ' ' + line
Line 173:         elif mode == 'info_return':

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

Itamar Heim has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3:

ping?

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

Adam Litke has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3: Code-Review-2

I think we should abandon this in favor of a YAML-based schema representation.  That would solve this and many other issues at the same time.

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

Saggi Mizrahi has posted comments on this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Patch Set 3:

go ahead and abandon.

Change in vdsm[master]: Detect optional parameter syntax errors in JSON schema

Itamar Heim has abandoned this change.

Change subject: Detect optional parameter syntax errors in JSON schema
......................................................................

Abandoned