Project

General

Profile

Actions

Feature #10014

closed

[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 about 8 years ago. Updated about 8 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
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 1 (0 open1 closed)

Task #10039: Review 10014-collection-error-detailResolvedLucas Di Pentima09/14/2016Actions
Actions #1

Updated by Ward Vandewege about 8 years ago

  • Description updated (diff)
Actions #2

Updated by Ward Vandewege about 8 years ago

  • Description updated (diff)
Actions #3

Updated by Ward Vandewege about 8 years ago

  • Description updated (diff)
Actions #4

Updated by Tom Morris about 8 years ago

  • Assigned To set to Tom Morris
Actions #5

Updated by Tom Clegg about 8 years ago

  • Assigned To changed from Tom Morris to Tom Clegg

10014-collection-error-detail @ f64b139

Actions #6

Updated by Tom Clegg about 8 years ago

  • Status changed from New to In Progress
Actions #7

Updated by Lucas Di Pentima about 8 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?

Actions #8

Updated by Tom Clegg about 8 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

Actions #9

Updated by Tom Clegg about 8 years ago

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

Updated by Lucas Di Pentima about 8 years ago

This LGTM, please merge.

Actions #11

Updated by Tom Clegg about 8 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:8cb96c1b23d02be8fac545d8c05a167454c1a11d.

Actions

Also available in: Atom PDF