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

Add support for xuetangx.com #412

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yiwenlu66
Copy link

🚨Please review the guidelines for contributing to this repository.

Proposed changes

Xuetangx (http://www.xuetangx.com/), based on OpenEdX, is the most popular MOOC platform in China. It would be fine to support it :-)

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here
to help! This is simply a reminder of what we are going to look for before
merging your code.

  • I have read the CONTRIBUTING doc
  • I agree to contribute my changes under the project's LICENSE
  • I have checked that the unit tests pass locally with my changes
  • I have checked the style of the new code (lint/pep).
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, please explain why you chose
the solution you did and what alternatives you considered, etc.

Reviewers

If you know the person who can review your code please add a @mention.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6ff43be on mcfloundinho:master into ** on coursera-dl:master**.

2 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6ff43be on mcfloundinho:master into ** on coursera-dl:master**.

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

Changes Unknown when pulling 6ff43be on mcfloundinho:master into ** on coursera-dl:master**.

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

Changes Unknown when pulling 8c47c43 on mcfloundinho:master into ** on coursera-dl:master**.

@coveralls
Copy link

coveralls commented Jan 19, 2017

Coverage Status

Changes Unknown when pulling b8c2684 on mcfloundinho:master into ** on coursera-dl:master**.

@iemejia
Copy link
Member

iemejia commented Jan 22, 2017

Hello, thanks, awesome job, Thanks a lot for contributing, and sorry if I took some time to review your changes.
Nice to add a new site for the tool, I want to ask you (if you have the patience) to fix some stuff, nothing big, just some things that make future maintenance easy, do you think we can iterate a bit to improve those small issues ?

@iemejia
Copy link
Member

iemejia commented Jan 22, 2017

One extra question have you tried your code on windows? We use to have tons of problems with chinese characters in windows, that's the reason why we ultra simplified everything to be ASCII. Anyway I think this solution is not future proof and maybe we must fix it as part of your PR (or I will do it in a future one).

@yiwenlu66
Copy link
Author

@iemejia Hello! I'm very pleased to improve my code or, if possible, help with the further maintenance of the project. I'll try coping with the windows encoding issue maybe later this week.

@iemejia
Copy link
Member

iemejia commented Jan 23, 2017

Awesome, this is great news, don't hesitate to ping me if you need anything. More hands that can help us improve the project are always welcome.

@yiwenlu66
Copy link
Author

I've tested with Windows and now it seems to work well (with Python 3).

image

@coveralls
Copy link

coveralls commented Jan 26, 2017

Coverage Status

Changes Unknown when pulling 7ab5045 on mcfloundinho:master into ** on coursera-dl:master**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7ab5045 on mcfloundinho:master into ** on coursera-dl:master**.

@coveralls
Copy link

coveralls commented Jan 26, 2017

Coverage Status

Coverage decreased (-3.7%) to 44.028% when pulling 9b7031b on yluthu:master into 265718c on coursera-dl:master.

@iemejia
Copy link
Member

iemejia commented Feb 1, 2017

Thanks, sorry I have not had many free time recently but I will try to check it ASAP, I will keep you posted, again sorry for the delay.

@rbrito
Copy link
Member

rbrito commented Dec 21, 2017

@mcfloundinho, can you please check which changes are needed now that edX has changed their structure? Do your proposed changes still work? Please, take a look at my changes in b6c77a6 in particular.

@ghost
Copy link

ghost commented Mar 16, 2018

@mcfloundinho
Hi, thanks for your code. With your xuetangx change, and @anmolsahoo25 's 403 fix, Xuetangx.com downloads fine, and works on windows.

Copy link
Member

@rbrito rbrito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase your changes to the current master branch?

Xuetangx, based on OpenEdX, is the most popular MOOC platform in China. It would be fine to support it :-)
add support for xuetangx.com.
Xuetangx has some malformed HTMLs, like this:

<a href=&coursera-dl#34; /c4x/TsinghuaX/70250023X/asset/LST0-1-0___________.zip&coursera-dl#34;>一个有趣应用-动画文件</a></pre><p></p></o1>

in http://www.xuetangx.com/courses/course-v1:TsinghuaX+70250023X_2015_2+sp/courseware/bfbdec0177a7466d9b6bc48c011ee401/63a0945fa22d486fa4e6976802f1f037/

from which the extracted url is 'http://www.xuetangx.com /c4x/TsinghuaX/70250023X/asset/LST0-1-0___________.zip',
and it's necessary to remove the space.
@yiwenlu66
Copy link
Author

@rbrito Yes, I've done the rebase. Sorry for the delay.

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.

None yet

4 participants