https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422015-01-07T19:42:34ZArvadosArvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=197362015-01-07T19:42:34ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/19736/diff?detail_id=18812">diff</a>)</li></ul> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=197632015-01-07T20:16:43ZTom Cleggtom@curii.com
<ul><li><strong>Target version</strong> changed from <i>Arvados Future Sprints</i> to <i>2015-01-28 Sprint</i></li></ul> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=197822015-01-07T20:37:34ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Brett Smith</i></li></ul> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=197862015-01-07T20:39:21ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Assigned To</strong> deleted (<del><i>Brett Smith</i></del>)</li></ul> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=197942015-01-07T20:42:28ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Peter Amstutz</i></li></ul> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=198762015-01-09T19:42:48ZBrett Smithbrett.smith@curii.com
<ul></ul><p>Reviewing <a class="changeset" title="4924: Refactor arv edit and arv create to improve error handling. * Error messages are now added..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/8295ba721133c341fd72350f7ca9438d1a99cdbe">8295ba7</a></p>
<ul>
<li>If the API server goes away, I get this message:<br /> <pre># Connection refused - connect(2) for "localhost" port 3030
# Please fix the error and try again.
</pre><br /> Most users probably won't be able to make any fix in this situation. I realize I'm breaking from the story a little bit here, but I wonder if this retry logic should only be used when there's an error in the user input, or the API server returns a well-formed error? In other cases, the old behavior of saying "I can't do that right now; I saved your edits here" seems more appropriate.</li>
<li>If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)</li>
<li>I think it could be extra helpful if the error message briefly described the category of error: "There was an error parsing your input," "The API server rejected your changes," etc. Some of the more specific error messages can be cryptic, so providing category information could help a user understand more quickly whether they broke the rules of JSON or Arvados.</li>
<li><code>arv edit</code> doesn't write any message if it successfully updates the object. Should it? That seems like it better follows the general UX patterns for these tools.</li>
<li>Not a blocker, just FYI, <code>any?</code> optionally takes a block, so you can write <code>.any? { condition }</code> rather than <code>.select { condition }.any?</code></li>
</ul>
<p>Thanks.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=198792015-01-09T20:36:15ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Brett Smith wrote:</p>
<blockquote>
<p>Reviewing <a class="changeset" title="4924: Refactor arv edit and arv create to improve error handling. * Error messages are now added..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/8295ba721133c341fd72350f7ca9438d1a99cdbe">8295ba7</a></p>
<ul>
<li>If the API server goes away, I get this message:<br />[...]<br />Most users probably won't be able to make any fix in this situation. I realize I'm breaking from the story a little bit here, but I wonder if this retry logic should only be used when there's an error in the user input, or the API server returns a well-formed error? In other cases, the old behavior of saying "I can't do that right now; I saved your edits here" seems more appropriate.</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li>If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)</li>
</ul>
</blockquote>
<p>I had this thought too, but the HTTP library documentation doesn't say how to get the status line (let me know if you find it: <a class="external" href="https://github.com/lostisland/faraday">https://github.com/lostisland/faraday</a>)</p>
<blockquote>
<ul>
<li>I think it could be extra helpful if the error message briefly described the category of error: "There was an error parsing your input," "The API server rejected your changes," etc. Some of the more specific error messages can be cryptic, so providing category information could help a user understand more quickly whether they broke the rules of JSON or Arvados.</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li><code>arv edit</code> doesn't write any message if it successfully updates the object. Should it? That seems like it better follows the general UX patterns for these tools.</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li>Not a blocker, just FYI, <code>any?</code> optionally takes a block, so you can write <code>.any? { condition }</code> rather than @.select { condition }.</li>
</ul>
</blockquote>
<p>Done.</p>
<p>Thanks.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=198832015-01-09T21:40:08ZBrett Smithbrett.smith@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>Brett Smith wrote:</p>
<blockquote>
<ul>
<li>If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)</li>
</ul>
</blockquote>
<p>I had this thought too, but the HTTP library documentation doesn't say how to get the status line (let me know if you find it: <a class="external" href="https://github.com/lostisland/faraday">https://github.com/lostisland/faraday</a>)</p>
</blockquote>
<p>Pfft. We can require net/http and ask the standard library for the normal message, at least:</p>
<pre>2.1.1 :037 > status = 422
=> 422
2.1.1 :038 > Net::HTTPResponse::CODE_TO_OBJ[status.to_s].to_s.sub(/^Net::HTTP/, "").titleize
=> "Unprocessable Entity"
</pre>
<p>I don't exactly like this code, but one grody line seems worthwhile to give the user <em>something</em> to work with.</p>
<p>The names <code>HttpResponse</code> and its <code>status</code> attribute make it seem like I'm dealing with an arbitrary HTTP response, rather than an exception and its message. Please consider a different name for the class (<code>ArvadosAPIError</code>?). And then I don't think you have to do anything else to get the messaging behavior you want:</p>
<pre>2.1.1 :040 > class BCSError < RuntimeError
2.1.1 :041?> end
=> nil
2.1.1 :042 > e = BCSError.new("foobar")
=> #<BCSError: foobar>
2.1.1 :043 > "#{e.class}: #{e}"
=> "BCSError: foobar"
</pre>
<p>Thanks.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=199112015-01-12T16:03:29ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Brett Smith wrote:</p>
<blockquote>
<p>Pfft. We can require net/http and ask the standard library for the normal message, at least:</p>
<p>[...]</p>
<p>I don't exactly like this code, but one grody line seems worthwhile to give the user <em>something</em> to work with.</p>
</blockquote>
<p>Done.</p>
<blockquote>
<p>The names <code>HttpResponse</code> and its <code>status</code> attribute make it seem like I'm dealing with an arbitrary HTTP response, rather than an exception and its message. Please consider a different name for the class (<code>ArvadosAPIError</code>?). And then I don't think you have to do anything else to get the messaging behavior you want:</p>
</blockquote>
<p>Done.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=199132015-01-12T16:41:51ZBrett Smithbrett.smith@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>Brett Smith wrote:</p>
<blockquote>
<p>Pfft. We can require net/http and ask the standard library for the normal message, at least:</p>
<p>[...]</p>
<p>I don't exactly like this code, but one grody line seems worthwhile to give the user <em>something</em> to work with.</p>
</blockquote>
<p>Done.</p>
</blockquote>
<ul>
<li>The <code>titleize</code> is on the wrong side of the parentheses. When the API server returns a full error, that gets titleized into something like "Properties Must Be A Hash, Not A Nil Class." Which Provides It With An Amusing Coat Of Inappropriate Formality.</li>
<li>Please move the <code>require</code> statement up near the top of the files with the others, for consistency.</li>
</ul>
<p>Other bugs found while I was kicking these tires:</p>
<ul>
<li>JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)</li>
<li>The successful update message for <code>arv edit</code> assumes that the UUID of the object hasn't been changed, but if I'm an admin I can totally do that. It should render the UUID from the object returned in the response, just like <code>arv create</code>.</li>
</ul>
<p>Thanks.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=199162015-01-12T17:23:21ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Brett Smith wrote:</p>
<blockquote>
<ul>
<li>The <code>titleize</code> is on the wrong side of the parentheses. When the API server returns a full error, that gets titleized into something like "Properties Must Be A Hash, Not A Nil Class." Which Provides It With An Amusing Coat Of Inappropriate Formality.</li>
</ul>
</blockquote>
<p>Fixed.</p>
<blockquote>
<ul>
<li>Please move the <code>require</code> statement up near the top of the files with the others, for consistency.</li>
</ul>
</blockquote>
<p>Fixed.</p>
<blockquote>
<p>Other bugs found while I was kicking these tires:</p>
<ul>
<li>JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)</li>
</ul>
</blockquote>
<p>Fixed.</p>
<blockquote>
<ul>
<li>The successful update message for <code>arv edit</code> assumes that the UUID of the object hasn't been changed, but if I'm an admin I can totally do that. It should render the UUID from the object returned in the response, just like <code>arv create</code>.</li>
</ul>
</blockquote>
<p>Fixed.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=199182015-01-12T18:26:18ZBrett Smithbrett.smith@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>Brett Smith wrote:</p>
<blockquote>
<ul>
<li>JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)</li>
</ul>
</blockquote>
<p>Fixed.</p>
</blockquote>
<p>Please collapse the two JSON rescue blocks in <code>check_result</code> to reduce duplicate code (<code>rescue JSON::ParserError, Oj::ParseError => e</code>) and then merge. Thanks.</p> Arvados - Bug #4924: [SDKs] Improve arv edit error handling UXhttps://dev.arvados.org/issues/4924?journal_id=199192015-01-12T18:40:09ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>67</i> to <i>100</i></li></ul><p>Applied in changeset arvados|commit:138a4b0a5de177faede5255841a5c6fca06b31f4.</p>