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

Git integration #137

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Git integration #137

wants to merge 18 commits into from

Conversation

jonas-t-s
Copy link
Contributor

This is a draft PR, since I'm currently working on the feature asked for in #46 and wanted to just note, that you can ask for changes or bring up your ideas, what I could improve.

At the current stage most planed functionality is not yet implemented, but at least a core function is implemented. An important todo is the documentation, which I still have to add.

The current state is, that I've implemented a git-repository for all courses, and after each call I've added a commit.

Anyway: I'm open to any suggestions/remarks.

@C0D3D3V
Copy link
Owner

C0D3D3V commented Feb 21, 2022

I would suggest making this completely optional feature. Because using git on PDFs, MP4s and co. increases the storage requirement drastically, usually by at least double. I would then directly make it so that GitPython is an optional dependency (I had that in mind for yt-dlp as well). For this we need to use lazy loading of the modules, I would just include lazy-import in the project for this, https://pypi.org/project/lazy-import/. To use it to load the git module only if needed. We can also build lazy loading ourselves with a 6 line, but I think lazy-import covers more error possibilities and so we are prepared for future lazy imports.

@C0D3D3V
Copy link
Owner

C0D3D3V commented Feb 21, 2022

As an alternative to lazy loading, we could also simply disable the functionality by a try except block if the module is not found.

Or we just add GitPython as a dependency, it's actually not a big dependency either, just unnecessary for the users who don't use it.

But the functionality itself should be configurable (i.e. deactivatable).

@C0D3D3V
Copy link
Owner

C0D3D3V commented Feb 21, 2022

https://github.com/C0D3D3V/Moodle-Downloader-2/blob/ab4560de165e2e7a0a54e8fda1d5c1cf8d94b9dd/moodle_dl/download_service/path_tools.py#L14
Please also make sure that to_valid_name() is applied to the course names, otherwise the repo folders might differ from the course folders.

@jonas-t-s
Copy link
Contributor Author

Yes, the configuration-thing is added to the todolist! As well as the search for a solution to the debendency.

For this we need to use lazy loading of the modules, I would just include lazy-import in the project for this, https://pypi.org/project/lazy-import/. To use it to load the git module only if needed. We can also build lazy loading ourselves with a 6 line, but I think lazy-import covers more error possibilities and so we are prepared for future lazy import
Yes that sounds interesting.

https://github.com/C0D3D3V/Moodle-Downloader-2/blob/ab4560de165e2e7a0a54e8fda1d5c1cf8d94b9dd/moodle_dl/download_service/path_tools.py#L14

Please also make sure that to_valid_name() is applied to the course names, otherwise the repo folders might differ from the course folders.

Ouch, that's a coding-error by myself. Thank you for stating that!

@jonas-t-s
Copy link
Contributor Author

jonas-t-s commented Feb 21, 2022

https://github.com/C0D3D3V/Moodle-Downloader-2/blob/ab4560de165e2e7a0a54e8fda1d5c1cf8d94b9dd/moodle_dl/download_service/path_tools.py#L14

Please also make sure that to_valid_name() is applied to the course names, otherwise the repo folders might differ from the course folders.
https://github.com/C0D3D3V/Moodle-Downloader-2/blob/main/moodle_dl/state_recorder/course.py#L8

Just checked the code again. If I understood it correctly, course.fullname is already applied to_valid_name() so no issue in using that, or have I misunderstood something?

@C0D3D3V
Copy link
Owner

C0D3D3V commented Feb 21, 2022

I had completely forgotten about this 😂, actually my goal was never to manipulate the data I get from moodle so that they are identical in the local database. Apparently I had changed my mind at one point :D Well then you can ignore that of course.

@C0D3D3V
Copy link
Owner

C0D3D3V commented Feb 21, 2022

Mh but we have to care about the
overwrite_name_with option ... https://github.com/C0D3D3V/Moodle-Downloader-2/blob/e634514e713cf9a9f8ac6b08c70a94c0e29569b2/moodle_dl/download_service/download_service.py#L134

Or we simply read the path from the saved_to path from each file. Since you have to handle the storage_path anyway. Since you want to execute the git commands in the storage_path.

@C0D3D3V
Copy link
Owner

C0D3D3V commented Feb 21, 2022

It is also questionable whether you really want to add all untracked files or only the changes that were passed in changed_courses (in the latter case you should also use changed_courses_to_notify instead, because it contains all changed files even after a fatal error). Since the user himself can make changes as he wants, but whether these should be tracked is his decision. Maybe you could make this as an option.

Also, files can change if they have changed online. moodle-dl does create a copy of the old file but overwrites the path of the old file with a new file. So you have to stage the modified files as well.

@C0D3D3V C0D3D3V force-pushed the main branch 2 times, most recently from c309031 to 47b3e62 Compare March 17, 2023 22:24
@sanjacob
Copy link

Be careful when using GitPython:

GitPython is not suited for long-running processes (like daemons) as it tends to leak system resources. It was written in a time where destructors (as implemented in the __del__ method) still ran deterministically.

https://gitpython.readthedocs.io/en/stable/intro.html#limitations

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.

3 participants