Code review comment for lp:~milo/linaro-patchmetrics/bug1013577

Revision history for this message
Milo Casagrande (milo) wrote :

> Thanks for the fixes:
>
> 161 # The request in this case needs to be a GET.
> 162 json_string =
> sanitize_json_response(urllib2.urlopen(request).read())
> 163 return json_string
> It is probably good to initialize json_string in gerrit.py to None here
> before we called sanitize_json_response.

Sure, fixed.

> 175 def sanitize_json_response(json_string):
> 176 """Cleans up the JSON response got from the REST interface, in order
> to be
> 177 valid JSON.
> 178
> 179 :param json_string: the JSON string to clean up.
> 180 """
> 181 # The JSON response is not valid JSON, there is a magic sequence at
> the
> 182 # beginning of the result. We are paranoid, and remove also other
> white
> 183 # spaces.
> 184 if json_string.startswith(GERRIT_SANITIZABLE_VALUE):
> 185 return json_string.lstrip(GERRIT_SANITIZABLE_VALUE).strip()
>
> Are we sure we that the json_string should always start with
> GERRIT_SANITIZABLE_VALUE ??
> If not, then we need to return json_string even when json_string does not
> start with GERRIT_SANITIZABLE_VALUE.

As for Gerrit documentation, it should be. Link where it is written is here:
https://gerrit-review.googlesource.com/Documentation/rest-api.html

But better handle that case too.
Thanks.

« Back to merge proposal