Make plugin non-intrusive: don't auto-set DOCUMENT_STORE_PYPATH#9
Make plugin non-intrusive: don't auto-set DOCUMENT_STORE_PYPATH#9jdesboeufs merged 3 commits intomainfrom
Conversation
- Remove DOCUMENT_STORE_PYPATH from preconfigure() - Keep SQLITE_DB_PATH default value in preconfigure() - Update README to clarify plugin must be explicitly activated - Plugin now requires explicit configuration to be used
There was a problem hiding this comment.
Pull Request Overview
This PR changes the plugin from automatically configuring itself to requiring manual configuration by users. The key modification removes the automatic registration of DOCUMENT_STORE_PYPATH from the preconfigure function, making users explicitly set this configuration parameter.
- Removed automatic
DOCUMENT_STORE_PYPATHsetting in thepreconfigurefunction - Updated documentation to reflect the manual configuration requirement
- Removed claims about "automatic registration" from feature list
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| addok_sqlite_store/init.py | Removed automatic setting of DOCUMENT_STORE_PYPATH in preconfigure function |
| README.md | Updated documentation to instruct users to manually configure DOCUMENT_STORE_PYPATH, added configuration examples |
Comments suppressed due to low confidence (1)
addok_sqlite_store/init.py:14
- The environment variable lookup is checking for 'SQLITE_DB_PATH', but according to the updated README, the environment variable should be 'ADDOK_SQLITE_DB_PATH'. This mismatch will cause the environment variable configuration method documented in the README to not work.
self.conn = sqlite3.connect(os.environ.get('SQLITE_DB_PATH') or config.SQLITE_DB_PATH)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ADDOK_SQLITE_DB_PATH is the recommended convention - SQLITE_DB_PATH is kept for backward compatibility - Priority: ADDOK_SQLITE_DB_PATH > SQLITE_DB_PATH > config.SQLITE_DB_PATH
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
addok_sqlite_store/init.py:51
- The
flushdbmethod usesconfig.SQLITE_DB_PATHdirectly, which doesn't respect the environment variable priority chain established in theinitmethod. This meansflushdbmay attempt to delete a different database file than the one currently being used. Store the resolveddb_pathas an instance variable ininit(e.g.,self.db_path) and use it here instead.
def flushdb(self):
os.unlink(config.SQLITE_DB_PATH)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Store db_path as instance variable to ensure flushdb deletes the actual database file being used, not just config.SQLITE_DB_PATH
Summary
Makes the
addok-sqlite-storeplugin non-intrusive by removing automatic activation when installed. The plugin now requires explicit configuration to be used.Problem
Previously, simply installing
addok-sqlite-storewould automatically set it as the document store backend via thepreconfigure()function. This caused issues in environments where:Changes
DOCUMENT_STORE_PYPATHfrompreconfigure(): The plugin no longer auto-activates itselfSQLITE_DB_PATHinpreconfigure(): Still provides a sensible default (addok.db) for the database pathDOCUMENT_STORE_PYPATHto activate the pluginMigration Guide
After this change, users need to explicitly configure the plugin:
Or via environment variable:
The
SQLITE_DB_PATHconfiguration remains optional with a default value ofaddok.db.Benefits