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

Add botpt #2

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add botpt #2

wants to merge 10 commits into from

Conversation

daxsoule
Copy link
Member

@daxsoule daxsoule commented Jan 3, 2020

Purpose for the request: blobs.py script hardwires the camhd container in the list_blobs function. I have modified the function from:
def list_blobs(storage_account, container):
blobs = []
blob_set, nextmarker = _list_blob_set('ooiopendata', 'camhd')
blobs = blobs + blob_set
while nextmarker:
blob_set, nextmarker = _list_blob_set('ooiopendata', 'camhd', nextmarker)
blobs = blobs + blob_set
return blobs

To the more general:
def list_blobs(storage_account, container):
blobs = []
blob_set, nextmarker = _list_blob_set('ooiopendata', container)
blobs = blobs + blob_set
while nextmarker:
blob_set, nextmarker = _list_blob_set('ooiopendata', container, nextmarker)
blobs = blobs + blob_set
return blobs

This allows the user to select any of the available containers in the 'ooiopendata' storage.

@tjcrone
Copy link
Member

tjcrone commented Jan 3, 2020

A good description for this PR might be: "This PR makes the ooiod list_blobs function more general." The listing of code is not very readable because it is not in code blocks, and also the code changes are in the PR in a nice readable form so this detail in the description is not necessary.

Also, I would exclude the changes to the notebook and to .gitignore from this PR, because these changes are not related. The changes to each of those files would fit better into their own PRs.

@@ -15,9 +15,9 @@ def _list_blob_set(storage_account, container, nextmarker = None):

def list_blobs(storage_account, container):
blobs = []
blob_set, nextmarker = _list_blob_set('ooiopendata', 'camhd')
blob_set, nextmarker = _list_blob_set('ooiopendata', container)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to also generalize ooiopendata to storage_account here and below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I see how did not need to include the code... and should have used the code blocks if I had. I will learn how to exclude files from a pull request for my next PR.

Copy link
Member

Choose a reason for hiding this comment

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

Roger that. This comment here was aimed at one line of code (the one highlighted). You can reply to the general comments in the main thread below.

@tjcrone
Copy link
Member

tjcrone commented Jan 3, 2020

I'm thinking it might be easier for you to go through this learning stage on a fork, since you can rewrite history on your fork without much if any disruption.

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