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

Add events for various operations #50

Closed
wants to merge 1 commit into from
Closed

Conversation

z0z0r4
Copy link

@z0z0r4 z0z0r4 commented Nov 30, 2024

#48

refer to QuickBackupM

Copy link

vercel bot commented Nov 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
prime-backup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 30, 2024 0:01am

@z0z0r4
Copy link
Author

z0z0r4 commented Nov 30, 2024

# events
BACKUP_DONE_EVENT 		= LiteralEvent('{}.backup_done'.format(PLUGIN_ID))  # -> source, backup_info
RESTORE_DONE_EVENT 		= LiteralEvent('{}.restore_done'.format(PLUGIN_ID))  # -> source, backup_info
DELETE_DONE_EVENT 		= LiteralEvent('{}.delete_done'.format(PLUGIN_ID))  # -> source, backup_info
EXPORT_DONE_EVENT 		= LiteralEvent('{}.export_done'.format(PLUGIN_ID))  # -> source, backup_info, file_path
IMPORT_DONE_EVENT 		= LiteralEvent('{}.import_done'.format(PLUGIN_ID))  # -> source, backup_info
TRIGGER_BACKUP_EVENT 	= LiteralEvent('{}.trigger_backup'.format(PLUGIN_ID))  # <- source, comment, operator
TRIGGER_RESTORE_EVENT 	= LiteralEvent('{}.trigger_restore'.format(PLUGIN_ID))  # <- source, backup_id, needs_confirm, fail_soft, verify_blob
TRIGGER_DELETE_EVENT 	= LiteralEvent('{}.trigger_delete'.format(PLUGIN_ID))  # <- source, [backup_id]
TRIGGER_EXPORT_EVENT 	= LiteralEvent('{}.trigger_export'.format(PLUGIN_ID))  # <- source, backup_id, export_format, fail_soft, verify_blob, overwrite_existing, create_meta
TRIGGER_IMPORT_EVENT 	= LiteralEvent('{}.trigger_import'.format(PLUGIN_ID))  # <- source, file_path, backup_format, ensure_meta, meta_override

Copy link
Contributor

@Fallen-Breath Fallen-Breath left a comment

Choose a reason for hiding this comment

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

Please do not simply copy designs from QuickBackupM. For PB, it's not a good design to trigger PB tasks via events

Also, please do not expose internal data structure contents with event arguments, and do not place MCDR stuffs outside prime_backup.mcdr packages

@Fallen-Breath
Copy link
Contributor

As said in #48 (comment), please provide the necessarity and scenarios of each events you added

@z0z0r4
Copy link
Author

z0z0r4 commented Nov 30, 2024

As said in #48 (comment), please provide the necessarity and scenarios of each events you added

我需要各个操作的事件,用于将每个操作映射到扩展插件,用于云备份

需要 import export create delete 事件,文档中推荐事件来跨插件通信,以及我认为不加入 restore 是很突兀的,没道理只增加 create done,所以我加了五个操作的十个对应事件

我认为这是必要的,不然根本不方便扩展。用 http 意味着得每个扩展都也得实现 http 通信,每一方的维护量都大增

我认为 event 也许并不难维护,除了在 task 结束后发送事件外,应该不会在未来的维护中造成困扰

文档中同样推荐用 event

For PB, it's not a good design to trigger PB tasks via events

我很好奇原因,如你所见改动量不大。或者应该触发更加底层的 api?

Also, please do not expose internal data structure contents with event arguments, and do not place MCDR stuffs outside prime_backup.mcdr packages

fixed

@Fallen-Breath
Copy link
Contributor

文档中推荐事件来跨插件通信

MCDR 的文档中说的是,“自定义事件是在插件之间传递消息的好办法”,并没有推荐使用事件进行跨插件通信

以及我认为不加入 restore 是很突兀的,没道理只增加 create done,所以我加了五个操作的十个对应事件

我不认为。请不要过度设计

我认为 event 也许并不难维护,除了在 task 结束后发送事件外,应该不会在未来的维护中造成困扰

我不认为。event 中参数的内容及可用性、event 发生时机的稳定性及可预期性,都是会为未来 PB 的维护和重构带来负担

@Fallen-Breath
Copy link
Contributor

若无法达成共识,此 PR 将不会被合并。建议去开个 fork 自行维护

@z0z0r4
Copy link
Author

z0z0r4 commented Nov 30, 2024

你的看法是什么...我从头到尾只得出了否定,你认为不应该加 event / 不应该加多个 event / 不应该给 event 加过多参数?

当文档中没有别的方法,且 mcdr 专门设计了 event 这个操作,我只能判断这对 mcdr 来说,是个推荐的方法

当前我只能不停提需求、降级,comment,像在答辩(并且我没啥水平),这样确实只能开 fork 了 =-=

如果你的看法是拒绝 event,我这就 close 然后 fork

@Fallen-Breath
Copy link
Contributor

你的看法是什么

我的看法已经在 #50 (review) 里给出了:“For PB, it's not a good design to trigger PB tasks via events”

我从头到尾只得出了否定

你不能理解我的回复,那我也只能否定

我只能判断这对 mcdr 来说,是个推荐的方法

请仔细阅读并理解 MCDR 文档说的内容。event 是用来派发事件的,并不适用于所有的场景。并且 MCDR 并未约束着插件只能用 event 来通讯

当前我只能不停提需求、降级

这也是你在 PR 前就应该避免的。若非小修小补,请先讨论清楚需求、双方确认实现方案了(注意本 PR 的描述部分提供的信息是非常匮乏),再去实现、交 PR。否则被开发者要求改来改去,浪费双方时间,是必然的结果

如果你的看法是拒绝 event

同上。我的看法已经在 #50 (review) 里给出了:“For PB, it's not a good design to trigger PB tasks via events”

@z0z0r4 z0z0r4 closed this Nov 30, 2024
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