fuo协议Show命令增强-Add Features #380
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cosven
reviewed
Jul 1, 2020
fuocore/cmds/show.py
Outdated
Comment on lines
51
to
61
| try: | ||
| song = provider.Song.get(sid) | ||
| except AttributeError: | ||
| return "No such provider : {}-{}".format(provider.name, provider_path_name) | ||
| # marshallow exception -- Validation Error | ||
| except Exception: | ||
| return "song identified by {} is unavailable in {} ".format(sid, provider.name) | ||
| else: | ||
| if song is None: | ||
| return "song {} is not in local library".format(sid) | ||
| return song |
cosven
reviewed
Jul 1, 2020
fuocore/cmds/show.py
Outdated
| except Exception: | ||
| return "song identified by {} is unavailable in {} ".format(sid, provider.name) | ||
| else: | ||
| if song is None: |
There was a problem hiding this comment.
如果 song 是 None，直接返回个 None 是不是就可以了？用 None 来表示 id 找不到对应的歌曲。
There was a problem hiding this comment.
啊的确如此，但是最终呈现在客户端的命令结果什么也没有，身为用户我可能不清楚该命令是否成功执行，所以选择了返回一个提示
|
非常感谢 ~ 今天下班后我会继续 review 下 ~ |
cosven
reviewed
Jul 2, 2020
| except NotFound: | ||
| # pass it to the exception handle procedure in __init__.py | ||
| raise | ||
| return None |
There was a problem hiding this comment.
这里只是 re-raise 了一下，看起来 try ... except... 不是很有必要？
There was a problem hiding this comment.
对，代码删改到后面，变成这一段只是为了使得__init__.py能捕获得异常的信息，所以就简单的re-raise，应该有更好的方法来着
| # use backstrace lib (the format_exc/print_exc) causes fail | ||
| # maybe the coroutine issue ? | ||
| uri_msg = "Bad Uri Exception: fuo://{}".format(self.rule) | ||
| return uri_msg |
There was a problem hiding this comment.
我觉得可以直接在抛错的时候把文字内容加进去，比如
raise NotFound(f"Bad Uri Exception: fuo://{self.rule}")| @@ -3,10 +3,22 @@ | |||
| from urllib.parse import parse_qsl, urlsplit | |||
|
|
|||
|
|
|||
| class NotFound(Exception): | |||
| class ShowCmdException(Exception): | |||
| @@ -66,6 +68,9 @@ def exec_cmd(cmd, *args, app): | |||
| playlist=playlist, | |||
| live_lyric=live_lyric) | |||
| rv = handler.handle(cmd) | |||
| except ShowCmdException: | |||
| logger.exception('handle cmd({}) error'.format(cmd)) | |||
There was a problem hiding this comment.
像这种已经被正确处理了的错误，打个 warning/error 级别就可以了。
打 exception 的话，一般是意料之外的错误。比如未知的错误等。
|
整体挺棒的！ 代码有个别地方可以稍微优化下，我等下 merge 之后再改下吧。 |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #317