-
-
Notifications
You must be signed in to change notification settings - Fork 328
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
[API] Escape ActionKeywords duplicates #3151
base: dev
Are you sure you want to change the base?
[API] Escape ActionKeywords duplicates #3151
Conversation
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe pull request introduces a modification to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)Flow.Launcher.Core/Plugin/PluginManager.cs (1)
This check effectively guards against duplicates. However, if Do you need a sample code snippet demonstrating thread-safe dictionary and list operations, along with null checks? Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
should we warn this with an error or something? I think technically plugin should call whether this actionkeyword is available before calling add (correct me if I am wrong as I add those thing way too long ago). |
We don't really support duplicate keyword...I don't know whether we should though |
I guess it should be mentioned in logs 🤔
This PR is not to add support, but to escape it in the first place. If there are 2 identical keywords, nothing bad happens |
I was a little bit not sure whether we want to do this. For example a plugin add two action keywords for two different purposes. However because of this change, when user delete one then both will be deactivated, which might not be an ideal scenario. Generally I think people solves this kind of thing with some reference counting, but I am not sure whether we want to do that. |
Probably separate conversation to this change. But to answer your question, currently I think we should allow duplicates. The result might look messy but this means having duplicate actionkeywords is not a show stopper. Thinking in the long term when we have hundreds of plugins it's just not feasible for plugin devs to find a non-used action keyword easily. |
🥷 Code experts: Yusyuriv, VictoriousRaptor Yusyuriv, jjw24 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
What is the current behaviour? IIRC flow will error out and disable the plugins right? With this change does it mean it will trigger both plugins' results after the action keyword is added? |
Stop. Let me get this straight to skip any misunderstandings. This PR is to escape per-plugin duplicates This does not interferes with other plugins. It only looks up current plugin before adding new action keyword to it P.S: I can rename PR & commit to be more specific if needed |
No I understand the purpose of this pr. However, as I mentioned, it impose the risk that when a plugin add 2 duplicate action keyword, but when it delete one, then both will be deleted. |
No description provided.