-
Notifications
You must be signed in to change notification settings - Fork 74
FC-0001: Remove EdxRestApiClient usage in edx-analatycis-data-api #542
FC-0001: Remove EdxRestApiClient usage in edx-analatycis-data-api #542
Conversation
Thanks for the pull request, @UvgenGen! I've created OSPR-6585 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
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.
Looks good to me. I'll send it over to the owning team for review!
Codecov Report
@@ Coverage Diff @@
## master #542 +/- ##
==========================================
+ Coverage 97.36% 97.51% +0.15%
==========================================
Files 61 61
Lines 3832 3826 -6
Branches 537 537
==========================================
Hits 3731 3731
+ Misses 71 65 -6
Partials 30 30
Continue to review full report at Codecov.
|
except AttributeError: | ||
logger.warning("LMS_BASE_URL is not configured! Cannot get video ids.") | ||
return None | ||
logger.info("Assuming the Course Blocks API is hosted at: %s", api_base_url) |
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 log seems unnecessary and noisy, although to be fair there is some more untouched noisy logging in here
'depth': 'all', | ||
'block_types_filter': 'video' | ||
} | ||
response = self.get(urljoin(api_base_url, 'blocks/'), params=blocks_kwargs) |
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 urljoin could just be rolled into the top urljoin
which points out that it isn't really api_base_url, it is url_of_this_blocks_call
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.
a couple of things I might like a tiny bit differently but then I read at the top that we're only using this client at the data API level for a specific fake course data command so they're not super important
@UvgenGen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
JIRA: DEPR-132
openedx/public-engineering#44