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

有点小bug #8

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

Conversation

luchenwei9266
Copy link

具体在readme里写了。不知道改得好不好

@codecov-io
Copy link

codecov-io commented May 27, 2019

Codecov Report

Merging #8 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #8   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          12     12           
=====================================
  Hits           12     12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d7ef25...ecd3e9c. Read the comment docs.

@HuangXiZhou HuangXiZhou self-requested a review May 30, 2019 07:54
descPageLink: 'https://www.dytt8.net/html/gndy/dyzz/20181204/57892.html'
[{ title: '2019年剧情战争《兰开斯特的天空》BD中英双字幕',
imgUrl: 'https://extraimage.net/images/2019/05/25/aea98d0038576db1158830d3c82cc888.jpg',
downloadLink: 'magnet:?xt=urn:btih:8dd1ccb0dedeaaacf78e4ae0d97bf88dfca8d060&dn=%e9%98%b3%e5%85%89%e7%94%b5%e5%bd%b1www.ygdy8.com.%e5%85%b0%e5%bc%80%e6%96%af%e7%89%b9%e7%9a%84%e5%a4%a9%e7%a9%ba.BD.720p.%e4%b8%ad%e8%8b%b1%e5%8f%8c%e5%ad%97%e5%b9%95.mkv&tr=udp%3a%2f%2ftracker.opentrackr.org%3a1337%2fannounce&tr=udp%3a%2f%2fthetracker.org%3a80%2fannounce&tr=http%3a%2f%2fretracker.telecom.by%2fannounce',
Copy link
Owner

Choose a reason for hiding this comment

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

我的理解是磁力链可以作为额外 config 参数传入

@@ -73,10 +73,13 @@ function handleGetMovieDetails(cheerio, uri, config) {
* @returns {Promise} movieDetails
*/
module.exports = (config) => {
config = config || { page: 1, include: DEFAULT_INCLUDE };
config = config || { page: 1, skip: 0, include: DEFAULT_INCLUDE };
Copy link
Owner

Choose a reason for hiding this comment

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

请问这个具体需求场景是什么呢

Copy link
Author

Choose a reason for hiding this comment

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

增加一个skip是希望可以考虑到前端展示数据时可以分页,这样前端展示的更方便的。同时,本身dytt的服务器是比较慢的,我觉得分页会更好。这样,前端只需要拿当前页的数据就行了,第2页只能第2页的数据,就不用再从第1页开始拿。

Copy link
Owner

Choose a reason for hiding this comment

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

懂了,这个没问题

@HuangXiZhou
Copy link
Owner

看起来没有特别大的问题,说下我的两点观点

  • 磁力链可以作为额外参数传入,不需要去改动原来的downloadUrl格式
  • 跳页的需求场景暂时不明确,望说明~

可以加个微信交流 18979122835 ;)

@luchenwei9266
Copy link
Author

看起来没有特别大的问题,说下我的两点观点

  • 磁力链可以作为额外参数传入,不需要去改动原来的downloadUrl格式
  • 跳页的需求场景暂时不明确,望说明~

可以加个微信交流 18979122835 ;)

一开始我在dytt那里看到的下载FTP地址是提示要下载迅雷的。我试着想在自己的Demo项目中利用windown.open('ftp://xxx')来实现调起迅雷下载,但实际上实现不了。所以我改成了磁力链接,这样在用户体验上,比起用户自己复制ftp地址到迅雷上再下载可能会更好。不过磁力链确实可以考虑一个额外参数传入,因为有时候ftp的速度可能会比磁力链更快。

@HuangXiZhou
Copy link
Owner

@luchenwei9266 有时间的话可以考虑改一下 pr,专门针对 skip 和磁力链即可

如果没有时间的话我之后稍作处理吧.. ;)

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

3 participants