Clean up function args #979
Conversation
| @@ -68,13 +66,12 @@ export default class CopilotPlugin extends Plugin { | |||
| // Always have one instance of sharedState and chainManager in the plugin | |||
| this.sharedState = new SharedState(); | |||
|
|
|||
| this.vectorStoreManager = new VectorStoreManager(this.app); | |||
| await this.vectorStoreManager.waitForInitialization(); | |||
There was a problem hiding this comment.
no need to explicitly block onload. The initialization will happen in the background and all public methods will wait for initialization.
|
|
||
| // Directly return the content assuming it's structured as expected | ||
| if (rewrittenQueryObject && "content" in rewrittenQueryObject) { | ||
| return rewrittenQueryObject.content; | ||
| return rewrittenQueryObject.content as string; |
There was a problem hiding this comment.
there is a type issue here but this refactor doesn't have any logic change. It should be safe to cast to string and keep the same type definition as before.
| private indexOps: IndexOperations; | ||
| private eventHandler: IndexEventHandler; | ||
| private initializationPromise: Promise<void>; | ||
| private lastKnownSettings: CopilotSettings | undefined; | ||
| public dbOps: DBOperations; |
There was a problem hiding this comment.
dbOps shouldn't be accessed before the initialization finishes
| @@ -26,11 +26,18 @@ export default class VectorStoreManager { | |||
| this.setupSettingsSubscription(); | |||
| } | |||
|
|
|||
| static getInstance(): VectorStoreManager { | |||
There was a problem hiding this comment.
made this singleton. It was only instantiated once
| }; | ||
|
|
||
| subscribeToSettingsChange(() => { | ||
| this.initializationPromise = reinitialize(); |
There was a problem hiding this comment.
we need to wait for initialization again when settings change causes the singleton to reinitialize
| @@ -121,15 +132,16 @@ export default class VectorStoreManager { | |||
| this.dbOps.onunload(); | |||
| } | |||
|
|
|||
| public async getOrInitializeDb(embeddingsAPI: Embeddings): Promise<Orama<any>> { | |||
There was a problem hiding this comment.
We shouldn't have another place to initialize DB. initialize method be the one that instantiate db
|
|
||
| constructor( | ||
| private dbOps: DBOperations, |
There was a problem hiding this comment.
hybridRetriever becomes a lot easier to use after taking out these singleton instance args
zeroliu
force-pushed
the
zero/fix-function-args
branch
from
4a52bd3 to
042726f
Compare
|
Nice! Thanks for the cleanup! ✅ |
Clean up constructor and function arguments when singleton can be used. See inline comments for more details
Tests
@vault,@web,@youtube,@pomodoroworks