-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implemented dependency injection for the data product endpoints #1
Conversation
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.
👍 Looks good to me! Reviewed everything up to 7b97240 in 6 seconds
More details
- Looked at
205
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. server/app/rest_api/routes/tests/test_data_products.py:63
- Draft comment:
The use ofIQuery | None
is unnecessary here since thequery
parameter is always provided. It should beIQuery
. This applies to line 77 as well. - Reason this comment was not posted:
Confidence changes required:50%
The use ofIQuery | None
in the test functions is unnecessary since thequery
parameter is always provided in the actual use case. It should beIQuery
.
Workflow ID: wflow_8MElyqCINbhM6cm4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 6 days left in your free trial, upgrade for $20/seat/month or contact us.
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.
👍 Looks good to me! Incremental review on f0361bb in 26 seconds
More details
- Looked at
190
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. server/app/rest_api/routes/tests/test_data_products.py:53
- Draft comment:
Thelist_usecase
function is defined as a synchronous function, but it should be asynchronous to match theIDataProductUsecases
interface.
def async list_usecase(query: GetPaginated) -> list[DataProductEntity]:
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Thelist_usecase
function is used in a test context and does not need to be asynchronous. The test is not actually calling the method asynchronously, so the suggestion to make it async is not necessary. The comment is not about a change made in the diff, but rather about the existing code structure in the test.
I might be overlooking the importance of consistency in the codebase, even in test mocks. However, since the test does not require asynchronous execution, the comment seems unnecessary.
While consistency is important, the primary goal of the test is to verify functionality, not to adhere to interface patterns. The test functions correctly without the async keyword.
The comment should be removed because it suggests an unnecessary change to a test mock function that does not need to be asynchronous.
2. server/app/rest_api/routes/tests/test_data_products.py:72
- Draft comment:
Theget_usecase
function is defined as a synchronous function, but it should be asynchronous to match theIDataProductUsecases
interface.
def async get_usecase(connector_id: str, query: GetByID) -> DataProductEntity:
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
TheIDataProductUsecases
interface has asynchronous methods, and theget_usecase
function in the test is synchronous. This mismatch could lead to issues if the interface expects asynchronous behavior. The comment seems to address a valid concern about interface compliance.
The test might be intentionally using a synchronous function for simplicity or testing purposes, and the interface compliance might not be necessary in this context. The comment might be assuming that the test needs to strictly adhere to the interface.
Even if the test is using a synchronous function for simplicity, it might still be beneficial to align with the interface to prevent potential issues or confusion in the future.
The comment addresses a valid concern about interface compliance and should be kept to ensure the test aligns with the asynchronous nature of theIDataProductUsecases
interface.
Workflow ID: wflow_qQQ2DPfXMXFKnKox
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 3 days left in your free trial, upgrade for $20/seat/month or contact us.
Summary:
Implemented dependency injection in
DataProductRoutes
usingIDataProductUsecases
andDataProductUsecases
, enhancing testability and flexibility.Key points:
DataProductRoutes
for data product endpoints.IDataProductUsecases
interface andDataProductUsecases
class for use case injection.DataProductRoutes
to acceptIDataProductUsecases
in constructor.usecases
calls withself._usecases
in methods.routes
initialization to useDataProductUsecases
.create_client
andcreate_usecases
helper functions for test setup.Generated with ❤️ by ellipsis.dev