-
Notifications
You must be signed in to change notification settings - Fork 640
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
Fixed the section parsing due to section name not being parsed properly #491
base: master
Are you sure you want to change the base?
Fixed the section parsing due to section name not being parsed properly #491
Conversation
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.
That doesn't work with me. I still have some kind of loop.
Could you please post the error message? |
There's no error message. I already posted what I have. |
Oh okay. Could you tell me the course link you are trying to download? |
Ofx: click here. I don't know why but it's downloading every course a lot of times. This time, it downloaded me 15 times the first 5 videos. |
Strange. Seems to be working for me. Does it create separate folders for the units? Or everything in one place? |
I guess I'd better provide you screenshots of the result. I downloaded it
in a new repo to avoid problems. I should precise that I'm on mac but my
Fedora is empty (still a lot to download in it to make it viable) and I
don't use my Windows desktop atm...
2018-03-12 15:14 GMT+01:00 Anmol Sahoo <[email protected]>:
… Strange. Seems to be working for me. Does it create separate folders for
the units? Or everything in one place?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#491 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AijnQ8KKK97Vf_TfFm08Ijejpon3ZIfsks5tdoKogaJpZM4Slo9N>
.
|
Would be great if you could share the screenshot? |
You mean sharing them on GH?
2018-03-16 8:25 GMT+01:00 Anmol Sahoo <[email protected]>:
… Would be great if you could share the screenshot?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#491 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AijnQ2hxJEu8NUZNhFEXOqliTKP6Bmf1ks5te2jSgaJpZM4Slo9N>
.
|
Btw, didn't you see the one I sent you via email?
#487
I added a couple of screenshots
2018-03-16 22:48 GMT+01:00 Gauthier Delvaulx <[email protected]>:
… You mean sharing them on GH?
2018-03-16 8:25 GMT+01:00 Anmol Sahoo ***@***.***>:
> Would be great if you could share the screenshot?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#491 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AijnQ2hxJEu8NUZNhFEXOqliTKP6Bmf1ks5te2jSgaJpZM4Slo9N>
> .
>
|
Hey. My bad. I saw the issue where you had posted the screenshots. Sorry about that. |
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.
OK. As soon as you fix the merge conflict, I will pull your request and merge it.
Thanks.
@@ -376,7 +376,7 @@ def _make_url(section_soup): # FIXME: Extract from here and test | |||
|
|||
def _get_section_name(section_soup): # FIXME: Extract from here and test | |||
try: | |||
return section_soup.div.h3.string | |||
return section_soup.find_all("h3", class_="section-title")[0].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.
@anmolsahoo25, is this still the case? Apparently, if I understood things correctly, there is some merge conflict with the recent changes that I pushed to the master branch...
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.
@rbrito , Yes. I am working on it. Will try to fix it ASAP. Pretty new to contributing on Github, so I'm just getting used to it.
🚨Please review the guidelines for contributing to this repository.
Proposed changes
Fixing the 0 courses available error.
Types of changes
Checklist
Further comments
Due to the new structure, the make title function was returning None. In turn, the filter for the properly parsed class was failing. I have tested my code for the EdX courses I am subscribed to, and it is working,
Reviewers
@rbrito