Add an addAllUserCourses option and fix a couple of bugs

RedEtherbloom commented 4 years ago

The base of this PR a new function addAllUserCourses that parses the users courses out of the login soup, so you don't have to lookup every ref_id if you want to download all your courses.

During testing I uncovered multiple inconsistencies between courses, that clashed with assumptions of the Downloader Code and crashed the program. I found three of these. I'll open separate issues for these later, and crosslink them. I tried to make the parsing an these places a bit more dynamic and robust to avoid similar problems in the future.

Hope I didn't touch you code too much, I tried to give it my best and make the PR as clean as possible for me. My commit history should be pretty verbose if you're unsure about what I intended with some functions.

Hope my PR will be helpful :) Oh and if you think that I did a good job, you can add the hacktoberfest-accepted label to my PR to help me with my Hacktoberfest progress

RedEtherbloom commented 4 years ago

PR should fix #22 #23 #24

jhelgert commented 4 years ago

At first sight it seems fine. I'll have a detailed look tomorrow. :)

jhelgert commented 4 years ago

PS: Could you please change the pull requests' branch to 'dev' instead of 'master'? While we're at it, I'll then clean up a few other parts of my old code - especially scanFolder().

RedEtherbloom commented 4 years ago

Sure, I can try to do that later. Hope this won't create too many merge conflicts.

So, about the design philosophy for my code that I mentioned: Ilias is imho often a pretty chaotic system, to a good part because of inconsistencies between courses. This meant that I wanted to make the code as robust as I can. This lead to the next conclusion: Maybe I want to contribute some tests to this project at some point. So I wanted to make the code easily testable.

This influenced how I wrote my code, split tasks into logical smaller methods, so that I'll be easily able to test them later. An example for this would be extractUserCourses and addAllUserCourses. The latter calls the former to get it's data, so I'll be able to just mock out the former and return a test-list, without having to write a login_soup for every test scenario.

Since you have some smaller functions like createIliasUrl in your code, I thought my code design should fit pretty good in there. I wanted to also make my methods a bit future proof. Sub tasks that might be useful in the future in one than more methods got there own. A (bad) example for this would be extractIdFromUrl.

Another nice advantage off this: Because this design exposes most of the internals the class can now also be used for other purposes than only being a download. It's now possible, for example, to log an instance in and use extractAllCourses to retrieve a list of all courses for other shenanigans the user may have in mind. I didn't really plan for this use case, but I at least didn't want to rule it out :)

Btw, thanks for the quick answer

jhelgert commented 4 years ago

TL;DR:

Fixing extractUserCourses() should be the highest prio.
My code design isn't really consistent and shouldn't serve as a rule.
(New) Rule of thumb: Once used one-liners shouldn't be an own method (createIliasUrl() & extractIliasId()).
If someone's willed to write tests, I'd agree on the refactoring into smaller methods as long as it isn't overkill.
I guess a good point to start might be scanFolder() where the two loops could be split into something like scanVideos() and scanForSubFolders().
Most of the past bugs were due to _parseFileProperties(), so refactoring (or at least commenting) this one might be a good idea.

jhelgert commented 4 years ago

Okay, lazy sunday, so I cleaned up the code, added a few tests and fixed #23 and #24. The new version is already on pip. If you still want to contribute, you can focus on addAllSemesterCourses().

PS: Just ping me back in case you still need the hacktoberfest-accepted label :).

RedEtherbloom commented 4 years ago

Sure, I'll focus on fixing that feature. It'll probably be a bit messy to port that over.

Also, I'd still really like the hacktoberfest-accepted label ^^ The system will only count it after the pull request is merged, so I won't get it too soon anyway. Having it now would give me some piece of mind ^^

jhelgert commented 4 years ago

Okay, lazy Tuesday morning (the last one for a while), so I implemented a first working version of addAllSemesterCourses() and added some tests, you can find it in the 0.5.0 branch.

I hope you don't mind since it's been your idea 🙈. I guess there a still some tests needed for all methods. Are you willed to focus on the tests? Would really appreciate it! And sorry for the mess!

RedEtherbloom commented 4 years ago

Oh, I actually really mind.

Working on this project has been a mess. Just for reference: I only published 50% of all the code I wrote, the rest was too redundant. I deleted it too not over clutter the project. But that was fine, I wanted to give this my all, to write the best code I can. I wanted to be as civil as possible.

Then you fixed #23 and #24 yourself, without telling me first. Okay, now 80% of my code was thrown away. I swallowed the frustration, because it wouldn't have been productive, I couldn't tell how you'd react, plus I really wanted to contribute something to this project.

Now 100% of my work got thrown away, you implemented the tests I talked about and now even implemented my own feature, after you told me "how about you focus on that?". And you did all of this without asking me beforehand. I didn't port the code to your new dev version yet, but what if I had started and completed it locally. In what situation would I have been? At that point 110% of my code would have been thrown away.

So yeah, I'm fuming. You can write those tests yourself, I'm not willing to dunkywork after you've trashed all of my code, after you've reaped all the fruits of my labour. Have a nice life, I don't think I'll contribute to this project any time soon.

jhelgert commented 4 years ago

You have the right to be annoyed. Communication is the key, and concerning this, I should have done much better. So I'm sorry about that. I want to comment on some points:

Then you fixed #23 and #24 yourself, without telling me first. [...] I swallowed the frustration because it wouldn't have been productive, I couldn't tell how you'd react, plus I really wanted to contribute something to this project.

It would be best if you told me. How should I have known? I got the impression I did you a favour, and you're okay with that as long as you'd get your hacktoberfest-accepted label. I can't see anything wrong with fixing bugs inside parts of my code.

Now 100% of my work got thrown away

Closing this PR throws away all of my work on reviewing this PR.

I didn't port the code to your new dev version yet, but what if I had started and completed it locally. In what situation would I have been? At that point, 110% of my code would have been thrown away.

In this case, I would have rejected my code to merge your code so that your work wasn't for nothing.

So yeah, I'm fuming. [...] after you've reaped all the fruits of my labour.

Tbh, I don't see where I reaped your fruits of labour. It's been your idea, but the code and addAllSemesterCourses() especially, was written by myself. Still, I intended to accept your PR for the hacktoberfest as a reward for your work.

RedEtherbloom commented 4 years ago

[Removed in agreement with jhelgert]

jhelgert commented 4 years ago

Just write me a mail so we can discuss the way forward. You can find my mail address inside the setup.py. 🤙🏻

RedEtherbloom commented 4 years ago

👍

jhelgert / IliasDownloaderUniMA

Add an addAllUserCourses option and fix a couple of bugs #21