-
-
Notifications
You must be signed in to change notification settings - Fork 525
feat: 统一所有模块获取时间操作, 增加Time模块统一管理时间相关操作, 在StarTools增加TimeTools模块, 提供接口以便使用和Core相同的时间(而不用维护一个时区设置) #1282
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
base: master
Are you sure you want to change the base?
Conversation
Original review guide in EnglishReviewer's Guide by SourceryThis pull request introduces a No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @anka-afk - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a method to
Time
to get the current date, in addition to the current datetime. - The
Time
class could benefit from a more detailed docstring explaining its purpose and usage.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -1,4 +1,4 @@ | |||
# What's Changed | |||
|
|||
- 修复 astrbot_updator 属性缺失与stt_enabled 未初始化 #252 |
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.
suggestion (typo): Add space after 与 character.
There should be a space after the '与' character for better readability.
Suggested implementation:
- 修复 astrbot_updater 属性缺失与 stt_enabled 未初始化 #252
If there are other similar occurrences in the changelog or project where the "与" character is used, please verify and add a space wherever needed for consistency.
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.
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
astrbot/core/star/star_tools.py:19
- [nitpick] The alias 'Time = Time' inside StarTools appears redundant. Consider removing it if the Time module can be directly used where needed.
Time = Time
另外感觉StarTools和Time做的有点太多了(?) 至少Time模块做的个人感觉功能有点多,个人感觉只让这个模块返回一个datetime对象会更好一点,不然这两个模块的重复度感觉就有点高。 以上仅为个人建议,仅供参考,现在的代码其实也能用。 |
我想的是给的Tools越详细越好的, 这样上手难度会比较低, 然后不需要让插件去调用AstrBot内部的一些模块, 把所有插件开发用到的相关模块全部塞进去, 然后AstrBot本体不会去使用这个模块里面的东西(里面的东西全部都是专门给插件开发使用的) |
有一说一在Time模块中加入更多功能确实能简化框架的部分调用。但以我个人风格来讲,更倾向于将更多功能封装到StarTools中,方便插件作者使用,而框架本身只调用一个简单返回datetime对象的Time模块,这样就可以避免框架里面出现功能/代码重复的两个不同用途的模块。或者把功能全放在Time模块,StarTools的TimeTools仅作为代理。 另外,感觉把Time更名为UnionTime(或者短一点,UniTime)表意更加清晰些?只改动一个大小写有的时候还是有一种在调用time的错觉( 不过刨除这些在风格上的意见不一致,剩下的就大概没什么问题了。 |
@Soulter 有时间来看看吗😿 |
我认为时间戳不应该加上时区偏移,它本质上代表从 Unix 纪元到现在的(毫)秒数。如果要加上时区也应该在被解释成人类可读时间时做转换。 |
并且现在存储层已经统一使用 UTC 时间戳,如果现在更换时间戳的定义,会引入破坏性更新。 |
此外,我认为统一成一个 Time 类的设计非常好 然后,为了保证前向兼容,建议保留 updator 包以及 AstrBotUpdator 类。(也就是,对于包来说,可以保留 updator.py 但是内部引用都引用 updater.py 的,然后 AstrBotUpdator 类作为 AstrBotUpdater 类的别名,参考 CommandResult) |
嗯, 所有涉及时间戳的地方都统一使用了UTC时间戳, 在需要format成字符串的地方才进行时区转换 |
这里也修改了, 但是感觉有点不太美观( |
Motivation
Modifications
Check
提交的时候才发现有这个规范, 这个commit message改起来太难了😭拼尽全力无法战胜
好的,这是翻译成中文的 pull request 总结:
Sourcery 总结
通过引入一个中心化的 Time 模块,统一各个模块中与时间相关的操作,从而提供一致的时区管理和时间相关实用程序。
新特性:
增强功能:
杂项:
Original summary in English
Summary by Sourcery
Unify time-related operations across modules by introducing a centralized Time module, providing consistent timezone management and time-related utilities
New Features:
Enhancements:
Chores: