Skip to content

update retrieval of datasets by jobs/tasks#204

Merged
jsh2134 merged 9 commits intomasterfrom
f-dataset-retrieve
Nov 22, 2017
Merged

update retrieval of datasets by jobs/tasks#204
jsh2134 merged 9 commits intomasterfrom
f-dataset-retrieve

Conversation

@jsh2134
Copy link
Copy Markdown
Contributor

@jsh2134 jsh2134 commented Nov 20, 2017

  • update retrieval to properly handle SolveClient
  • make them properties
  • add dataset id shortcut also
  • add shortcuts to get Dataset vault_object
  • add shortcuts to get Object vault and Object parent

@jsh2134 jsh2134 requested a review from davecap November 21, 2017 14:45
Comment thread solvebio/resource/dataset.py Outdated
def vault_object(self):
from solvebio import Object
response = self._client.get(
os.path.join(Object.class_url(), self['vault_object_id']), {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use a urljoin.

Comment thread solvebio/resource/datasetcommit.py Outdated

@property
def dataset_id(self):
return self['dataset']['id']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised there isn't a dataset_id attribute on this object... or is it write only?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... ya weird. that is there already. removing

Comment thread solvebio/resource/datasetcommit.py Outdated
@property
def dataset(self):
self._client.Dataset.retrieve(self['dataset'])
response = self._client.get(self['dataset']['url'], {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is dataset['url'] a relative URL or absolute?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolute

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is ok!

Comment thread solvebio/resource/datasetmigration.py Outdated

@property
def source(self):
if not self.source_id:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize this was possible

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it raise an exception? return None?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya....you are correct...not sure why i did that. serializers have required=True. removing

if not self.source_id:
return

response = self._client.get(self['source']['url'], {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above... is it abs. or relative url?

Comment thread solvebio/resource/object.py Outdated
""" Returns the parent object """
if self['parent_object_id']:
response = self._client.get(
os.path.join(self.class_url(), self['parent_object_id']), {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urljoin

Comment thread solvebio/resource/object.py Outdated
""" Returns the vault object """
from . import Vault
response = self._client.get(
os.path.join(Vault.class_url(), str(self['vault_id'])), {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urljoin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess technically you could do Vault.retrieve(id, client=self._client) also

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ya, thats cleaner. will do.

Copy link
Copy Markdown
Contributor

@davecap davecap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

response = self._client.get(
os.path.join(self.class_url(), self['parent_object_id']), {})
return convert_to_solve_object(response, client=self._client)
return self.retrieve(self['parent_object_id'], client=self._client)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since retrieve is a classmethod, should this be Object.retrieve?

Comment thread solvebio/resource/datasetcommit.py Outdated
def dataset(self):
self._client.Dataset.retrieve(self['dataset'])
response = self._client.get(self['dataset']['url'], {})
return convert_to_solve_object(response, client=self._client)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it might be sufficient to do:

return convert_to_solve_object(self['dataset'], client=self._client)?

Comment thread solvebio/resource/datasetimport.py Outdated

@property
def dataset_id(self):
return self['dataset']['id']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is dataset_id here too?

@jsh2134 jsh2134 merged commit 863fec4 into master Nov 22, 2017
@jsh2134 jsh2134 deleted the f-dataset-retrieve branch November 22, 2017 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants