Skip to content

Slash-Support : supporting '/' within upload/download for files in bundles#266

Merged
amarjandu merged 41 commits intomasterfrom
slash-support
Mar 9, 2019
Merged

Slash-Support : supporting '/' within upload/download for files in bundles#266
amarjandu merged 41 commits intomasterfrom
slash-support

Conversation

@amarjandu
Copy link
Copy Markdown
Contributor

@amarjandu amarjandu commented Mar 7, 2019

Supporting / within upload/download for files in bundles

Address the need to support nested folders/files within bundle
structures.
Imaging data bundles will have specifc file structures that neeed to
be preserved the same when being uploaded or downloaded to the DSS.
This also addresses a workaround for the matrix team where ! was
being used due to not having slash-support

Resolves: #261
See also: HumanCellAtlas/data-store#1885

amarjandu and others added 30 commits February 25, 2019 13:59
@amarjandu amarjandu requested a review from DailyDreaming March 7, 2019 01:08
Copy link
Copy Markdown

@mikebaumann mikebaumann left a comment

Choose a reason for hiding this comment

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

Thanks, Amar!

Copy link
Copy Markdown

@mikebaumann mikebaumann left a comment

Choose a reason for hiding this comment

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

Amar, please add a unit test that actually uses the command line interface (not just the Python bindings), to both verify this fix and prevent similar similar errors occurring in the future. Thanks!

@amarjandu amarjandu requested a review from mikebaumann March 7, 2019 20:51
Comment thread hca/cli.py Outdated
Comment thread hca/cli.py Outdated
Comment thread hca/cli.py Outdated
Copy link
Copy Markdown

@mikebaumann mikebaumann left a comment

Choose a reason for hiding this comment

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

Hi @amarjandu, please address the comments I added .... thanks!

Comment thread hca/cli.py Outdated
Comment thread hca/cli.py Outdated
parser = get_parser()

if len(sys.argv) < 2:
if len(args) < 2:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test used to be performed using sys.argv, which includes the script name as sys.argv[0], but args does not include the script name, so this:

if len(args) < 2

should be changed to:

if len(args) < 1

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.

i added this change but needed to also perform

 if not args:
       args = sys.argv[1:]

this resolves a parsing issue with the script name

Comment thread test/integration/dss/test_dss_cli.py Outdated
Comment thread hca/dss/util/__init__.py Outdated
@mikebaumann
Copy link
Copy Markdown

@amarjandu Assuming you are going to squash and merge this, please:

  • Add a more descriptive title to the PR
  • A more complete description of the change in the PR
  • A link to the associated issue
    Thanks!

Comment thread hca/dss/util/__init__.py Outdated
Comment thread hca/dss/util/__init__.py
Copy link
Copy Markdown

@mikebaumann mikebaumann left a comment

Choose a reason for hiding this comment

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

Added another comment, still reviewing ...

Comment thread hca/dss/__init__.py
@amarjandu amarjandu changed the title Slash support Slash-Support : supporting '/' within upload/download for files in bundles Mar 8, 2019
Copy link
Copy Markdown

@mikebaumann mikebaumann left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

Comment thread hca/cli.py
def main(args=None):
if '--help' in sys.argv or '-h' in sys.argv:
if not args:
args = sys.argv[1:]
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.

@DailyDreaming i had to do this to get around parser issue when trying to use the cli after performing a make install everything works within testing + manual cli usage. Can you think of a reason it would break a parser later on?

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.

No, I think that was the correct call.

Copy link
Copy Markdown
Contributor

@DailyDreaming DailyDreaming left a comment

Choose a reason for hiding this comment

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

LGTM.

@amarjandu amarjandu merged commit 078d407 into master Mar 9, 2019
@DailyDreaming DailyDreaming deleted the slash-support branch March 9, 2019 00:48
maniarathi pushed a commit that referenced this pull request Mar 28, 2019
…ndles (#266)

Supporting / within upload/download for files in bundles

Address the need to support nested folders/files within bundle
structures.
Imaging data bundles will have specifc file structures that neeed to
be preserved the same when being uploaded or downloaded to the DSS.
This also addresses a workaround for the matrix team where ! was
being used due to not having slash-support

Resolves: #261
See also: HumanCellAtlas/data-store#1885
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.

5 participants