Updated to continue working in 2025, refactored codebase #8
Conversation
|
Hi @aaroexxt! Thanks for the PR! I will review soon. I don't plan to maintain this library in the long-term, so I will see if a student org takes the repo. If you are interested on helping to maintaining it, I would be happy to known :) |
benjavicente
left a comment
benjavicente
left a comment
There was a problem hiding this comment.
The only required changes would be to revert where there was the Google Drive download functionality and to fix my name :)
There is a lot of improvements that I will comment on issues, but all come form the fact that I wasn't really experienced on making good code back then 😅
| def __get(self, query: str, **kwarg): | ||
| """Performs a GET request to the Canvas API with 250 items per page.""" | ||
| # Extract existing parameters if provided | ||
| params = kwarg.pop("params", {}) | ||
|
|
||
| # Ensure 'per_page' is set to 500 | ||
| params["per_page"] = 500 |
There was a problem hiding this comment.
I thing that's better for the per_page param to be defined per query, some endpoints might restrict the page size if I remember correctly.
Also, if you want to add a default to the params, it might be better to do something like this:
def __get(self, query: str, *, params = {"per_pagee": 500}, **kwarg):Right now that implementation seems to override what was passed to __get
| pages_api_url = f"https://{self.domain}/api/v1/courses/{course_id}/pages" | ||
| response = requests.get( | ||
| pages_api_url, | ||
| headers={"Authorization": f"Bearer {self.token}"}, | ||
| params={"per_page": 250} # Ensuring 250 pages per request | ||
| ) |
There was a problem hiding this comment.
This ideally should use the __get method.
| if self._is_canvas_url(item["external_url"]): | ||
| self._download_canvas_page(course_id, item["external_url"], course_name) |
There was a problem hiding this comment.
Here there was a method to download files from Google Drive, since at UC Chile it was common for courses to link to files there. I would keep the get_external_download_url check, or at least add a comment to say that here is where one could add a custom downloader for external services.
| Complete file downloader for Canvas (Instructure)! | ||
|
|
||
| Features: | ||
| Originally by [Ben Javicente](https://github.com/benjavicente/canvas-file-downloader), essentially rewritten from scratch by [Aaron Becker](https://github.com/aaroexxt). |
There was a problem hiding this comment.
| Originally by [Ben Javicente](https://github.com/benjavicente/canvas-file-downloader), essentially rewritten from scratch by [Aaron Becker](https://github.com/aaroexxt). | |
| Originally by [Benjamin Vicente](https://github.com/benjavicente/canvas-file-downloader), essentially rewritten from scratch by [Aaron Becker](https://github.com/aaroexxt). |
Benja is the spanish equivalent of Ben in english ;)
2 participants
New features: