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

✨ 更新数据库模型 #39

Closed
wants to merge 18 commits into from
Closed

✨ 更新数据库模型 #39

wants to merge 18 commits into from

Conversation

GuGuMur
Copy link
Owner

@GuGuMur GuGuMur commented Dec 17, 2023

No description provided.

@GuGuMur GuGuMur marked this pull request as ready for review December 31, 2023 01:47
@GuGuMur GuGuMur requested a review from AzideCupric December 31, 2023 01:47
Comment on lines 39 to 41
- uses: actions/checkout@v4
- name: Set up PDM
uses: pdm-project/setup-pdm@v3
Copy link
Collaborator

Choose a reason for hiding this comment

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

为什么要改这里,不就是上面的./.github/actions/setup-python离得内容吗

Copy link
Owner Author

Choose a reason for hiding this comment

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

不知道为什么action全报错了,完整抄下一份发现能用

Copy link
Collaborator

Choose a reason for hiding this comment

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

感觉是你修改了哪个地方的问题(

Copy link
Owner Author

Choose a reason for hiding this comment

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

这个pr之前commit的action test报错记录出错位置

不是很懂导入怎么出错了,反正是把他展开写进去以后能用了

Comment on lines 45 to 50
- name: Install dependencies
run: |
pdm sync -d -G test
- name: Run Tests
run: |
pdm run -v pytest tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

有什么区别吗,为什么要sync一次?以前不是没问题

Copy link
Owner Author

Choose a reason for hiding this comment

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

抄的 pdm 文档里的(

然后之前的 pr 没问题 但是这个 pr 里每次 commit 以后的 preview action 就全炸了

Comment on lines 28 to 35
- repo: https://github.com/pre-commit/mirrors-prettier
rev: v4.0.0-alpha.3
hooks:
- id: prettier
types_or: [javascript, jsx, ts, tsx, markdown, yaml, json]
exclude: "admin-frontend/"
stages: [commit]

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里保留是考虑到还可以留着格式化yaml和md,就没删,删掉exclude那段就行

Copy link
Owner Author

Choose a reason for hiding this comment

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

最近的一次错误报告:对flag怎么写没什么头猪,直接删了(逃

Copy link
Collaborator

Choose a reason for hiding this comment

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

怪事 也没写这两个参数啊

> 注意:非[超级用户](https://nonebot.dev/docs/appendices/config#superusers)只可删除自己绑定的账号,超级用户可以删除bot数据库内所有账号
> 超级用户可以删除bot数据库内的任意账号
>
> [OneBot V11](https://onebot.adapters.nonebot.dev/) 适配器中的 QQ 群管理员以上权限的用户可以在群内删除本群内的任意账号
Copy link
Collaborator

Choose a reason for hiding this comment

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

我认为在一个好的权限插件出来之前还是保留原状?这样也太平台特化了

cred: Mapped[str] = mapped_column(String, doc="森空岛账号CRED")
token: Mapped[str] = mapped_column(String, doc="森空岛账号TOKEN", nullable=True)
note: Mapped[str] = mapped_column(String, doc="备注", nullable=True, unique=True)
note: Mapped[str] = mapped_column(String, doc="备注", nullable=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

备注得保证唯一性吧,不然有个人备注abc, 另一个人也备注abc,那 signin abc 签到谁的

Copy link
Owner Author

Choose a reason for hiding this comment

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

signin abc以后bot会返回当前用户能控制的所有备注为abc的账户,通过回复序号来确定签到哪个

Comment on lines 19 to 24
if not token:
return "未绑定"
else:
if is_group:
return "已绑定"
return token
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not token:
return "未绑定"
else:
if is_group:
return "已绑定"
return token
if not token:
return "未绑定"
elif is_group:
return "已绑定"
return token

@AzideCupric
Copy link
Collaborator

往一个pr里塞进太多不同方向的更改了,显得很杂,得考虑拆分
但拆分成若干pr的话现在的发版操作会导致版号疯涨,我先改改发版操作

@AzideCupric
Copy link
Collaborator

AzideCupric commented Jan 4, 2024

这个pr里糅合了多个不同方向的更改,应该拆分开来

  • 数据库字段添加,以及添加导致的命令修改
  • 旧命令的改动
  • CI 变动(已经修过了)

@GuGuMur
Copy link
Owner Author

GuGuMur commented Jan 6, 2024

一开始这个 pr 的灵感来源就是群友想 update 备注发现没权限 然后转过来要烤我,新数据库和新指令实际都是为了这个服务的,CI 更新其实是看靠前的 commit 出错不顺眼的(

简单来说就是感觉没必要拆这个 pr

@AzideCupric
Copy link
Collaborator

AzideCupric commented Jan 6, 2024

这明显两个不重叠的更改吧(
发现卡了一些reviews

help_text="在私聊绑定一个在群聊中添加的签到账号",
),
Subcommand(
"rebind",
Copy link
Collaborator

Choose a reason for hiding this comment

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

不应该叫 rebind,其实就是添加了一个 send_to 字段,直接叫 sendto 就行

Copy link
Owner Author

Choose a reason for hiding this comment

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

其实指令叫 send_to 感觉更容易混(

Subcommand(
"list",
help_text="列出所有签到账号",
),
Subcommand(
"del",
Args["identifier", str],
Args["identifier", str]["position?", int],
Copy link
Collaborator

Choose a reason for hiding this comment

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

增加position容易导致误删吧,万一在增删账号之后原来那个账号的位置改变了呢,我觉得不应该加

Copy link
Owner Author

Choose a reason for hiding this comment

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

发送指令以后能处理的列表就固定在 state 里了,position也只对这部分起效

Copy link
Collaborator

Choose a reason for hiding this comment

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

发送指令以后能处理的列表就固定在 state 里了,position也只对这部分起效

何意 没太看懂(

help_text="使用uid或者备注删除一个签到账号",
),
Subcommand(
"update",
Args["identifier", str],
Args["identifier", str]["position?", int],
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的position也是

Subcommand(
"signin",
Args["identifier", str],
Args["identifier", str]["position?", int],
Copy link
Collaborator

Choose a reason for hiding this comment

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

还有这里的position

await skland.finish("该UID已经被注册,请检查")
new_record = SklandSubscribe(user=user_account.dict(), uid=uid, token=token, cred="", note=note)
# 1. 检查UID是否被注册
async with db_session.begin():
Copy link
Collaborator

Choose a reason for hiding this comment

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

一定需要这个 async with 吗

Copy link
Owner Author

Choose a reason for hiding this comment

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

好像是直接用过一次之后就不能再直接用了,所以套了几层begin

Comment on lines 71 to 73
result: list[SklandSubscribe] = (await db_session.scalars(stmt)).all()
result = [i for i in result if compare_user_info(i.user, event_session_dict)]
if result:
await skland.finish("该note已经被您注册,请检查")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
result: list[SklandSubscribe] = (await db_session.scalars(stmt)).all()
result = [i for i in result if compare_user_info(i.user, event_session_dict)]
if result:
await skland.finish("该note已经被您注册,请检查")
result: list[SklandSubscribe] = (await db_session.scalars(stmt)).all()
if any(i for i in result if compare_user_info(i.user, event_session_dict)):
await skland.finish("该note已经被您注册,请检查")

不过我觉得 compare_user_info 的实现很怪, 能不能直接比较没有dict()之前的值

Copy link
Owner Author

Choose a reason for hiding this comment

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

试过很多种从数据库里带函数筛的方法,全用不了

db_session: AsyncSession = Depends(get_session),
):
is_group = skland_is_group(bot, event)
all_subscribes: list[SklandSubscribe] = await skland_list_subscribes(bot, event, matcher, db_session)
Copy link
Collaborator

Choose a reason for hiding this comment

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

skland_list_subscribes为什么不直接依赖注入呢

Copy link
Collaborator

Choose a reason for hiding this comment

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

并且在内部判断权限来决定返回的数据

Copy link
Owner Author

Choose a reason for hiding this comment

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

呜呜呜
前面的commit有试过这种的,但是prompt值就写不到state里了,所以拿了这么一种很丑的写法

Copy link
Collaborator

Choose a reason for hiding this comment

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

?感觉不对劲 哪个commit

Copy link
Owner Author

Choose a reason for hiding this comment

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

能想到的一个commit

没记错的话,执行到询问position那一步会报错 keyerror 找不到 prompt

@AzideCupric AzideCupric force-pushed the database_update branch 2 times, most recently from 5a2088d to bd31d4e Compare January 12, 2024 14:36
@GuGuMur
Copy link
Owner Author

GuGuMur commented Jan 20, 2024

这回不拆分是不行咯(

@GuGuMur GuGuMur closed this Jan 20, 2024
@AzideCupric
Copy link
Collaborator

@AzideCupric
Copy link
Collaborator

动工之前 先写个 Feature Issue 吧

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.

2 participants