update import and create-dataset shortcuts to require full path #217

jsh2134 · 2017-12-20T21:48:26Z

update the upload and create-dataset shortcuts to require a dataset full path (breaking change that removes --vault and --path) from those commands
add validation code to objects and vaults and add some incomplete path inference

davecap

Let's discuss tomorrow.

davecap · 2017-12-22T01:00:27Z

README.md

@@ -234,11 +234,11 @@ remained unchanged.
 The `objects` property of a resource has been renamed `solve_objects`.

 6.  The `import` and `create-dataset` command-line utilities now require
-`--vault` and `--path` arguments.  The `dataset` argument (`test-dataset`
+`--dataset_full_path` arguments.  The `dataset` argument (`test-dataset`


isn't it --dataset-full-path ? maybe it should just be --full-path?

davecap · 2017-12-22T01:00:39Z

README.md

 below) no longer can contain slashes.

 ```
-create-dataset --capacity=small --vault=test --path=/  test-dataset
+create-dataset --capacity=small --dataset_ful_path=acme:test:/examples test-dataset


Looks like a typo.

davecap · 2017-12-22T01:02:44Z

solvebio/cli/main.py

@@ -103,8 +90,8 @@ class SolveArgumentParser(argparse.ArgumentParser):
                            'Options are "append" (default) or "overwrite".'
                },
                {
-                    'name': 'dataset_name',
-                    'help': 'The name of the dataset'
+                    'name': 'dataset_full_path',


Maybe you should use 'flags': 'dataset-full-path'? How about just full-path?

if we use flags, then what would the default be? or can you make flags required?

davecap · 2017-12-22T01:02:55Z

solvebio/cli/main.py

@@ -158,26 +127,20 @@ class SolveArgumentParser(argparse.ArgumentParser):
                            'medium (<500M), large (>=500M)'
                },
                {
-                    'name': 'dataset_name',
-                    'help': 'The name of the dataset'
+                    'name': 'dataset_full_path',


Same as above.

davecap · 2017-12-22T01:03:30Z

solvebio/cli/main.py

-                    'help': 'The name of the vault to use when '
-                            'creating a new dataset (via --create-dataset), '
-                            'defaults to your personal vault',
-                },
                {
                    'flags': '--path',


Maybe for consistency with other command line functions, this should also just be --full-path?

im fine with that.

davecap · 2017-12-22T01:07:23Z

solvebio/test/test_object.py

+        test_cases = [
+            ['{0}:myVault'.format(domain), '{0}:myVault:/'.format(domain)],
+            ['acme:myVault', 'acme:myVault:/'],
+            ['myVault', '{0}:/myVault'.format(user_vault)],


I think we may want to raise an error in this case since it is pretty ambiguous...

See here: https://github.com/solvebio/api.solvebio.com/pull/1583/files#diff-5e6e2bc0f160dfe279ad0a8638bc0ba0R63

davecap · 2017-12-22T01:07:44Z

solvebio/test/test_object.py

+            ['acme:myVault', 'acme:myVault:/'],
+            ['myVault', '{0}:/myVault'.format(user_vault)],
+            ['acme:myVault:/uploads_folder', 'acme:myVault:/uploads_folder'],
+            ['acme:myVault:/uploads_folder', 'acme:myVault:/uploads_folder'],


davecap · 2017-12-22T01:08:21Z

solvebio/test/test_object.py

+            ['myVault:/uploads_folder', '{0}:myVault:/uploads_folder'.format(domain)],  # noqa
+            ['/uploads_folder', '{0}:/uploads_folder'.format(user_vault)],
+            [':/uploads_folder', '{0}:/uploads_folder'.format(user_vault)],
+            ['myVault/uploads_folder', '{0}:/myVault/uploads_folder'.format(user_vault)],  # noqa


raise error here too? We do that thing where if there is no slash it raises an error.

davecap · 2018-01-02T16:04:54Z

solvebio/cli/data.py

-        vault_name = args.vault
-    else:
-        vault_name = Vault.get_personal_vault().name
+        raise Exception(


didnt you remove this flag?

davecap · 2018-01-02T16:05:07Z

solvebio/cli/data.py

-    else:
-        vault_name = Vault.get_personal_vault().name
+    if args.path:
+        raise Exception(


davecap · 2018-01-02T16:05:30Z

solvebio/cli/data.py

+    #
+    if args.vault:
+        raise Exception(
+            '[Deprecated] --vault has been deprecated. Pass vault path as part of "full_path"'  # noqa


* refactor full path validators with regex * fix issues with regexes and overrides; use explicit function names * switch to tilde for personal vault

jsh2134 · 2018-01-11T15:14:33Z

solvebio/cli/data.py


    """
+    # FIXME: Does this need to be here? What about other commands?


I think remove this?

jsh2134 added 9 commits December 20, 2017 16:07

use dataset_full_paths

88e010e

fix test

c30974a

use full paths in upload command as well

0e9e15a

rename vars/func, add vault path validator

8a634e4

typo

c30d187

add tests

7afec18

update readme

60bf5a7

python3 syntax

87055b4

remove print

6e5a2eb

jsh2134 requested review from jhuttner and davecap December 22, 2017 00:37

davecap requested changes Dec 22, 2017

View reviewed changes

jsh2134 added 7 commits December 28, 2017 16:07

standardize path variable to full-path/full_path and add deprecations

e36c0f3

update messages

37b2134

update cases

2151143

remove deprecation

549b1aa

remove except statement that masked already handled error

5fb70d5

update path handling for creating datasets and errors

9d43da2

including these deprecations is very confusing

4e0b745

davecap reviewed Jan 2, 2018

View reviewed changes

add back --vault and --path, but ensure --full-path overrides

d8328d1

davecap approved these changes Jan 8, 2018

View reviewed changes

try refactoring path validators with regex (#218)

c3c4383

* refactor full path validators with regex * fix issues with regexes and overrides; use explicit function names * switch to tilde for personal vault

jsh2134 commented Jan 11, 2018

View reviewed changes

solvebio/cli/data.py Outdated

"""

# FIXME: Does this need to be here? What about other commands?

Copy link

Contributor Author

jsh2134 Jan 11, 2018

Choose a reason for hiding this comment

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

I think remove this?

davecap added 2 commits January 11, 2018 10:23

update readme

26abed7

support backwards compatible create-dataset command

a6e4fd7

jsh2134 merged commit ee76f7d into master Jan 11, 2018

jsh2134 deleted the f-update-import-shortcuts branch January 11, 2018 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update import and create-dataset shortcuts to require full path #217

update import and create-dataset shortcuts to require full path #217

jsh2134 commented Dec 20, 2017 •

edited

davecap left a comment

davecap Dec 22, 2017

davecap Dec 22, 2017

davecap Dec 22, 2017

jsh2134 Dec 28, 2017

davecap Dec 22, 2017

davecap Dec 22, 2017

jsh2134 Dec 28, 2017

davecap Dec 22, 2017

davecap Dec 22, 2017

davecap Dec 22, 2017

davecap Dec 22, 2017

davecap Jan 2, 2018 •

edited

davecap Jan 2, 2018

davecap Jan 2, 2018

jsh2134 Jan 11, 2018


		"""
		# FIXME: Does this need to be here? What about other commands?

update import and create-dataset shortcuts to require full path #217

update import and create-dataset shortcuts to require full path #217

Conversation

jsh2134 commented Dec 20, 2017 • edited

davecap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecap Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsh2134 commented Dec 20, 2017 •

edited

davecap Jan 2, 2018 •

edited