Skip to content

Conversation

@isatotun
Copy link

This is a basic working piece for the upload and download of blob files into and from azure.
The methods here will be extended but for now in terms of testing some development pipelines this should suffice.

@isatotun isatotun requested a review from mcarans April 29, 2024 21:43
@github-actions
Copy link

Test Results

97 tests  ±0   97 ✅ ±0   9s ⏱️ -1s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 9d4cb9b. ± Comparison against base commit ce79080.

@coveralls
Copy link

Coverage Status

coverage: 97.02%. remained the same
when pulling 9d4cb9b on add-azure-client-logic
into ce79080 on main.

Copy link
Contributor

@mcarans mcarans left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've left a few comments for you to consider.

account: str,
container: str,
key: str,
data: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be helpful to users of the function specify a type hint eg. Optional[Dict] if it can be None or a dictionary

account (str): Storage account to access the blob
container (str): Container to download from
key (str): Key to access the blob
blob (str): Name of the blob to be downloaded. If empty, then it is assumed to download
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default is None, the type should be Optional[str]

"If-None-Match": "",
"If-Unmodified-Since": "",
"Range": "",
"CanonicalizedHeaders": "x-ms-date:"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using f-strings for this and other cases of concatenating strings

@isatotun
Copy link
Author

isatotun commented May 2, 2024

I am going to put this PR on hold for now and getting back to it next month.

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.

4 participants