-
Notifications
You must be signed in to change notification settings - Fork 3
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
DAS-1894 #5
DAS-1894 #5
Conversation
@@ -22,6 +22,13 @@ __pycache__/ | |||
|
|||
# Generated files | |||
stash | |||
downloaded_granules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
download_granules
saves a file into the current directory, so I added these file types to the .gitignore
from unittest import TestCase | ||
from unittest.mock import patch | ||
from unittest import TestCase, mock | ||
from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops I don't use MagicMock
will remove for the next commit
@@ -45,6 +49,7 @@ def test_with_concept_id(self, granule_query_mock): | |||
('foo') | |||
granule_query_mock.return_value.get.assert_called_once_with(10) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need two spaces and realized that it's PEP8
@@ -62,13 +63,41 @@ def get_granules(concept_id: str = None, | |||
return granule_response | |||
|
|||
|
|||
def get_granule_link(query_response: list) -> str: | |||
def get_granule_link(granule_response: list) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this name was better
return granule_link | ||
|
||
|
||
def get_granule_name(granule_response: list) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really just a helper function for getting the granule filename, so we don't have to go through the granule_response
''' Use the requests module to download data via https. | ||
''' | ||
# Check if a string was entered for input parameters | ||
if isinstance(granule_link, str) and isinstance(out_filename, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided go with checking if granule_link
and out_filename
are strings. I won't be offended if you think we should do this differently.
@@ -91,3 +91,12 @@ class MissingGranuleDownloadLinks(CustomError): | |||
def __init__(self, download_link): | |||
super().__init__('MissingGranuleDownloadLinks', | |||
f'No links for granule record: {download_link}') | |||
|
|||
|
|||
class RequestsException(CustomError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomError
for the W 😎
|
||
|
||
def _mock_requests(self, status=200, content="CONTENT"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I am pretty proud of this one. I wanted to mock requests.get
and figured out it would be easier to write a function that returns this mock object, instead of creating the mocked object in the same test for checking the mocked object against download_granules
. Also, I noticed in some of your harmony code you start functions that are only used once with a _
?? I was reading somewhere that this is convention but feel free to correct me if I am wrong.
if isinstance(granule_link, str) and isinstance(out_filename, str): | ||
f = open(out_filename, 'w+') | ||
else: | ||
raise ValueError('Not a string') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I should make this error message more meaningful.
Description
Added functionality for downloading a granule locally from CMR
Jira Issue ID
DAS-1894
Local Test Steps
PR Acceptance Checklist
CHANGELOG.md
updated to include high level summary of PR changes.VERSION
updated if publishing a release.