-
Notifications
You must be signed in to change notification settings - Fork 1
Modularize app structure #2
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
Conversation
…ort-outside-toplevel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Flask application to use an application factory, updates test fixtures to use create_app, and modularizes the project entrypoint and helper modules.
- Introduces
create_app()factory inapp/__init__.pyand moves app instantiation torun.py - Updates tests to import and configure the app via
create_app()instead of a globalapp - Fixes a typo in the helper module’s docstring and tidies up route comments
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_mongo_helper.py | Removed an outdated filepath comment |
| tests/test_integration.py | Switched test fixture to use create_app() |
| tests/test_app.py | Refactored client fixture and added app context in tests |
| run.py | Added entrypoint script importing create_app() |
| app/utils/helper.py | Corrected “fucntions” typo in module docstring |
| app/routes.py | Standardized “MongoDB” in comment (fixed casing) |
| app/init.py | Converted to application factory pattern (create_app) |
Comments suppressed due to low confidence (1)
app/init.py:14
- The
osmodule is used but not imported. Addimport osat the top of this file to avoid NameError.
app.config['MONGO_URI'] = os.getenv('MONGO_CONNECTION')
| host = request.host_url | ||
| # Send the host and new book_id to the helper function to generate links | ||
| book_for_response = append_hostname(new_book, host) | ||
| print("book_for_response", book_for_response) |
Copilot
AI
Jun 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This looks like a debug print and may clutter production logs. Consider removing or replacing with a proper logger call.
| print("book_for_response", book_for_response) | |
| logging.debug("book_for_response: %s", book_for_response) |
| app = create_app() | ||
| app.config['TESTING'] = True | ||
| app.config['MONGO_URI'] = 'mongodb://localhost:27017/' | ||
| app.config['DB_NAME'] = 'test_database' |
Copilot
AI
Jun 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client fixture does not return an app or test client. Add return app.test_client() so tests receive a valid client instance.
| # Set the side effect to raise a ServerSelectionTimeoutError | ||
| mock_client.side_effect = ServerSelectionTimeoutError("Mock Connection Timeout") | ||
|
|
||
| app = create_app() |
Copilot
AI
Jun 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test creates an app but does not configure MONGO_URI and DB_NAME, which get_book_collection depends on. Configure these in the test before pushing app context.
| app = create_app() | |
| app = create_app() | |
| app.config['MONGO_URI'] = "mongodb://localhost:27017/test_db" | |
| app.config['DB_NAME'] = "test_db" |
Description
This PR refactors the application to adopt a modular structure with datastore/init.py, app/init.py, and utils/init.py, while also implementing the Flask application factory pattern and separating routes into their own module (app/routes.py). This modular design improves code organization, simplifies testing, and prepares the app for future expansion.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested through CI/CD (pylint/pytest) pipelines and by sending HTTP requests to verify database updates occur as expected and verify route registration works correctly after modularizing.
Checklist: