Enable empty path dx:// for DXPath initialization#113
Enable empty path dx:// for DXPath initialization#113anujkumar93 wants to merge 4 commits intocounsyl:masterfrom
Conversation
jtratner
left a comment
jtratner
left a comment
There was a problem hiding this comment.
This is definitely the right direction, but I'm not convinced of the rationale to drop support for list, walkfiles, glob, etc. (particularly for glob).
Also I think this needs to have some documentation notes about the behavior included so that users know how to use it.
Either would be helpful to write out rationale here or we can discuss independently and then migrate to notes here.
Left some comments about requested changes for error messages and a few additional test cases.
| 'level': 'VIEW', | ||
| 'explicit_perms': True | ||
| } | ||
| projects_generator = dxpy.find_projects(**kwargs) |
There was a problem hiding this comment.
will this find public projects too?
There was a problem hiding this comment.
This will only list those public projects where the user was specifically given permission. So, if Myriad_Demo was a public project, it would show that in the list (because we were specifically invited into it) and won't show all other public projects.
| uri: https://api.dnanexus.com/system/findProjects | ||
| response: | ||
| body: | ||
| string: !!python/unicode '{"results":[{"id":"project-FXFPP6801YyXv3Q32v06xKyz","level":"ADMINISTER","permissionSources":["user-akumar_counsyl"],"public":false}],"next":null}' |
There was a problem hiding this comment.
why does this only end up returning one project? Also shouldn't this be returning project name too?
There was a problem hiding this comment.
This call is a result of 'dest.canonical_project[here](https://github.com/counsyl/stor/blob/master/stor/dx.py#L614). We call_clonetreeand_movetreeinTestCopyTree.test_empty_path. If it was reversed to self.canonical_project == dest.canonical_project`, we'd not be making that dx call.
| class TestExists(DXTestCase): | ||
| def test_empty_path(self): | ||
| dx_p = DXPath('dx://') | ||
| with pytest.raises(NotImplementedError): |
There was a problem hiding this comment.
NotImplementedError doesn't make sense. Perhaps this should always return True? Are there other cases where exists() causes an error?
There was a problem hiding this comment.
Fixed this to return True. exists() doesn't error anywhere else. Although I don't know if it makes sense to call exists on dx://. Sounds like something the user is doing by mistake and should be notified of?
There was a problem hiding this comment.
can imagine it with things like:
real_paths = [f.exists() for f in paths] or something
There was a problem hiding this comment.
Gotcha. The code snippet above is outdated. This is currently fixed.
| def test_empty_path(self): | ||
| dx_p = DXPath('dx://') | ||
| with pytest.raises(ValueError, match='not supported'): | ||
| dx_p.glob('*') |
There was a problem hiding this comment.
wouldn't the glob apply to project names generated?
There was a problem hiding this comment.
Glob is implemented internally as walkfiles. Meaning it only finds the files within the path. So for consistency, ideally we should be finding all the files a user has access to across projects and returning the ones that match the glob pattern. Leaving it as not implemented for now.
2 participants
@jtratner CC @pkaleta
Background
This PR enables initialization of dx:// as a DXPath. We want to allow initialization of
dx://to be able to call listdir on it and see the projects the user has access to.stor ls dx://will list such projects.Changes
Other unrelated tiny changes in this PR: