Skip to content
This repository has been archived by the owner on Jun 30, 2019. It is now read-only.

add xiami provider #23

Merged
merged 5 commits into from
Aug 21, 2018
Merged

add xiami provider #23

merged 5 commits into from
Aug 21, 2018

Conversation

cosven
Copy link
Owner

@cosven cosven commented Aug 17, 2018

by @cyliuu

Copy link
Owner Author

@cosven cosven left a comment

Choose a reason for hiding this comment

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

整体非常棒,标记了一些需要小幅修改的地方。

PR 的计划

  • 这次先把基本的一些功能加入
  • 一些比较高级的功能:获取相似歌曲、更新播放列表信息、排行榜等功能暂时不加入,相关代码也之后在加入

}
payload = self.encrypt_request(data)
res_data = self.request("GET", action, payload)
if res_data is None:
Copy link
Owner Author

@cosven cosven Aug 17, 2018

Choose a reason for hiding this comment

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

最好不要自动刷新 cookie,在上层来完成这个操作。

现在这样写有几个问题:

  1. 重复代码:每次请求都有可能出现 cookies 过期的情况,也就是每个函数都需要写这么一段逻辑
  2. 多个请求容易出现未知情况,不利于排查问题,一般来说,尽量避免
  3. 底层应该尽量简单,只完成自己的功能,上层来处理复杂的逻辑

}
payload = self.encrypt_request(data)
res_data = self.request("GET", action, payload)
if res_data is None:
Copy link
Owner Author

Choose a reason for hiding this comment

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

同上

}
payload = self.encrypt_request(data)
res_data = self.request("GET", action, payload)
if res_data is None:
Copy link
Owner Author

Choose a reason for hiding this comment

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

同上

data = {'songId': song_id}
payload = self.encrypt_request(data)
res_data = self.request("GET", action, payload)
if res_data is None:
Copy link
Owner Author

Choose a reason for hiding this comment

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

同上

songs = []
try:
# FIXME: 这里写个 for 循环应该有点问题
for start in range(0, len(song_ids), 200):
Copy link
Owner Author

@cosven cosven Aug 17, 2018

Choose a reason for hiding this comment

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

这里在 for 循环中发送 web 请求,很容易出问题:比如 IP 被禁、一个请求出错,整个就失败了

logger.error(str(e))
return None

def album_comments(self, album_id):
Copy link
Owner Author

Choose a reason for hiding this comment

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

同样的,可以拆成两个函数

}
payload = self.encrypt_request(data)
res_data = self.request("GET", action, payload)
if res_data is None:
Copy link
Owner Author

Choose a reason for hiding this comment

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

同上,不要在这里自动刷新 cookie

logger.error(str(e))
return [], None

def artist_comments(self, artist_id):
Copy link
Owner Author

Choose a reason for hiding this comment

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

拆成两个

return []

# 相似歌曲 相似歌单 相似歌手
def similar_songs(self, song_id):
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: similar 相关的接口,这次先不加

return []

# 创建歌单 更新歌单名称 删除歌单
def new_playlist(self, name='default'):
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: playlist 的一些更新操作暂时不加

@coveralls
Copy link

coveralls commented Aug 20, 2018

Coverage Status

Coverage increased (+3.8%) to 57.513% when pulling b9178a2 on cyliuu-pr-xiami into 2dbe60e on master.

@cosven cosven merged commit d58d54a into master Aug 21, 2018
@cosven cosven deleted the cyliuu-pr-xiami branch August 21, 2018 15:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants