Feature #10014

[SDKs] Python SDK should be more forthcoming when a file can't be found in a collection: list the path that can't be found

Added by Ward Vandewege over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Start date:
09/14/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

I propose a patch along these lines:

diff --git a/sdk/python/arvados/collection.py b/sdk/python/arvados/collection.py
index 56d8b23..0722f28 100644
--- a/sdk/python/arvados/collection.py
+++ b/sdk/python/arvados/collection.py
@@ -802,7 +802,7 @@ class RichCollectionBase(CollectionBase):
         if isinstance(source, basestring):
             source_obj = source_collection.find(source)
             if source_obj is None:
-                raise IOError(errno.ENOENT, "File not found")
+                raise IOError(errno.ENOENT, "File not found", source)
             sourcecomponents = source.split("/")
         else:
             source_obj = source

Subtasks

Task #10039: Review 10014-collection-error-detailResolvedLucas Di Pentima

Associated revisions

Revision 8cb96c1b
Added by Tom Clegg over 4 years ago

Merge branch '10014-collection-error-detail' closes #10014

History

#1 Updated by Ward Vandewege over 4 years ago

  • Description updated (diff)

#2 Updated by Ward Vandewege over 4 years ago

  • Description updated (diff)

#3 Updated by Ward Vandewege over 4 years ago

  • Description updated (diff)

#4 Updated by Tom Morris over 4 years ago

  • Assigned To set to Tom Morris

#5 Updated by Tom Clegg over 4 years ago

  • Assigned To changed from Tom Morris to Tom Clegg

10014-collection-error-detail @ f64b139

#6 Updated by Tom Clegg over 4 years ago

  • Status changed from New to In Progress

#7 Updated by Lucas Di Pentima over 4 years ago

Grepping the raise statements on collection.py, I've found some additional spots that may be improved:
  • Line 310: Would be useful to add the intended manifest stream that cannot be added because of containing a whitespace?
  • Line 625: Read-only collection name can be added to the error message.
  • Line 738: We refer to a “subcollection” not being empty, wouldn’t be more consistent to name it “directory” or “subdirectory” just like in other parts of the code?
  • Line 818: Can target path and/or source be added to that ArgumentError message?

Also, a question related to find_or_create: If the file or collection exist and is not of create_type, shouldn’t we raise some exception instead of blindly return it without checking?

#8 Updated by Tom Clegg over 4 years ago

Lucas Di Pentima wrote:

  • Line 310: Would be useful to add the intended manifest stream that cannot be added because of containing a whitespace?

Added.

  • Line 625: Read-only collection name can be added to the error message.

Not actually sure how to get the collection name/uuid/pdh here...

  • Line 738: We refer to a “subcollection” not being empty, wouldn’t be more consistent to name it “directory” or “subdirectory” just like in other parts of the code?

Yes, updated.

  • Line 818: Can target path and/or source be added to that ArgumentError message?

Hm. The target path is empty by definition, and the intent seems to be to complain only if we don't have a string representation of source.

If I look closer, this function just starts to look rather buggy. sourcecomponents[-1] crashes before we even get to this error if source wasn't a basestring. Maybe investigate this in a separate branch?

Also, a question related to find_or_create: If the file or collection exist and is not of create_type, shouldn’t we raise some exception instead of blindly return it without checking?

The docstring claims this is the desired behavior but it does look like some callers are making different assumptions. Separate branch?

→commit:c727716

#9 Updated by Tom Clegg over 4 years ago

  • Category set to SDKs
  • Target version set to 2016-09-28 sprint

#10 Updated by Lucas Di Pentima over 4 years ago

This LGTM, please merge.

#11 Updated by Tom Clegg over 4 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:8cb96c1b23d02be8fac545d8c05a167454c1a11d.

Also available in: Atom PDF