Repositories

<ojarjur@google.com>
[email protected] wants to merge refs/heads/ojarjur/improve-show into refs/heads/master
2015-11-11 22:33:04

Usability improvements for showing reviews:

  1. Include all of the details when displaying a review; in particular:
    1. The submitted status.
    2. The latest continuous-integration status.
    3. The latest static analysis results.
  2. Include a snippet of the preceding code when displaying a review comment.
<ckerur>
ckerur commented on 2015-11-11 23:28:32 with status ℹ️

Overall Looks good.

<ckerur>
ckerur commented on 2015-11-11 23:28:32 with status ℹ️
Location { commit: Some("a3f462b139c47923e69b34b7c43cf63d3866560d"), path: Some("commands/output/output.go"), range: Some(Range { start_line: Some(83) }) }

Can an exception be thrown here ?

<ckerur>
ckerur commented on 2015-11-11 23:28:32 with status ℹ️
Location { commit: Some("a3f462b139c47923e69b34b7c43cf63d3866560d"), path: Some("commands/output/output.go"), range: Some(Range { start_line: Some(56) }) }

this method is a little confusing to read.

Consider changing into 2 methods -one that calculates status and the other that does the printing ...

That way this method could look like

if(r.Resolved == nil and r.Submitted) return "tbr" if(r.Resolved == nil) return "pending"

if (*r.Resolved ){ if(r.Submitted) { return "submitted" } else return "rejected" }

if(r.submitted) return "danger"...

<ojarjur@google.com>
[email protected] commented on 2015-11-12 17:43:33 with status ℹ️
Location { commit: Some("a3f462b139c47923e69b34b7c43cf63d3866560d"), path: Some("commands/output/output.go"), range: Some(Range { start_line: Some(83) }) }

No, no exceptions can come out of this. The method is meant to be safe for any string; if it sees one that it doesn't know how to handle, then it just leaves it alone.

<ckerur>
ckerur commented on 2015-11-12 18:07:51 with status ℹ️
Location { commit: Some("cf2857001dc0ad09db95bbd61931aa41f6128cb2"), path: Some("commands/output/output.go"), range: Some(Range { start_line: Some(88) }) }

Just repeating..what happens if an exception is thrown here ? How does Go handle it ?

<ojarjur@google.com>
[email protected] commented on 2015-11-12 18:19:50 with status ℹ️
Location { commit: Some("cf2857001dc0ad09db95bbd61931aa41f6128cb2"), path: Some("commands/output/output.go"), range: Some(Range { start_line: Some(88) }) }

Go does not have special control flow for expcetions the way Java (for instance) does. Since this method does not declare error as part of its return type, the only way it can not return a string is if the entire program exits

<git-mirror>
git-mirror commented on 2015-11-12 20:43:21 with status ℹ️
Location { commit: Some("a3f462b139c47923e69b34b7c43cf63d3866560d"), path: Some("commands/output/output.go"), range: Some(Range { start_line: Some(56) }) }
<ckerur>
ckerur commented on 2015-11-12 22:13:56 with status 👍