Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1590: Enable listing directories #1832

Merged
merged 4 commits into from
Jan 24, 2017
Merged

Fix #1590: Enable listing directories #1832

merged 4 commits into from
Jan 24, 2017

Conversation

troydai
Copy link
Contributor

@troydai troydai commented Jan 24, 2017

No description provided.

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

A few minor questions and a suggestion but LGTM.

@@ -71,7 +71,8 @@ def transform_file_output(result):
more clearly distinguishes between files and directories. """
new_result = []

for item in result.get('items', [result]):
iterable = result if isinstance(result, list) else result.get('items', result)
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning value from the custom command could be a list instead of a ListGenerator. That is case when directories are filtered.

- name: --num-results
type: integer
short-summary: Specifies the maximum number of directories to return. The default value is
5000. The value cannot be less or equal to zero.
Copy link
Member

Choose a reason for hiding this comment

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

If this is used as the input to the SDK call to get files and directories, then this is not accurate help text because any files (which will be filtered out) would count against the 5000 count quota.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Then this parameter cannot be accurately functional.

Copy link
Member

Choose a reason for hiding this comment

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

Arguably you could simply omit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you like the idea to hide the option on both az storage file list and az storage directory list until we have a good paging story?

@@ -163,6 +173,10 @@
helps['storage file list'] = """
type: command
short-summary: List files and directories in the specified share.
parameters:
- name: --files-only
Copy link
Member

Choose a reason for hiding this comment

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

Consider something like '--omit-directories' or '--suppress-directories'. For '--file-only' I would need to read the help text to know what that means. The alternative are (at least to mean) a little more self-explanatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

1. Remove two parameters from both file list and directory list to
simplify the command.
2. Update help doc.
@tjprescott tjprescott merged commit 06e1d7a into Azure:master Jan 24, 2017
@troydai troydai deleted the fix-1590 branch January 24, 2017 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants