-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(sqlstore): add sqlstore package #6835
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 f3f3157 in 1 minute and 21 seconds
More details
- Looked at
1460
lines of code in25
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. pkg/factory/servicetest/http.go:19
- Draft comment:
NewHttpService
should be renamed toNewHTTPService
to follow Go naming conventions for acronyms. - Reason this comment was not posted:
Confidence changes required:50%
TheNewHttpService
function should follow Go naming conventions, which suggest using camel case for acronyms. Therefore,NewHttpService
should be renamed toNewHTTPService
. This change will improve code readability and maintain consistency with Go naming conventions.
2. pkg/factory/servicetest/http.go:26
- Draft comment:
Name
method should returnfactory.MustNewName(service.name)
to follow Go naming conventions for acronyms. - Reason this comment was not posted:
Confidence changes required:50%
TheNewHttpService
function should follow Go naming conventions, which suggest using camel case for acronyms. Therefore,NewHttpService
should be renamed toNewHTTPService
. This change will improve code readability and maintain consistency with Go naming conventions.
3. pkg/factory/servicetest/http.go:13
- Draft comment:
httpService
should be renamed toHTTPService
to follow Go naming conventions for acronyms. - Reason this comment was not posted:
Confidence changes required:50%
TheNewHttpService
function should follow Go naming conventions, which suggest using camel case for acronyms. Therefore,NewHttpService
should be renamed toNewHTTPService
. This change will improve code readability and maintain consistency with Go naming conventions.
4. pkg/factory/servicetest/http.go:1
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable inpkg/factory/servicetest/http.go
. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_CzV5zkacbQ067KUY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 35ecd38 in 22 seconds
More details
- Looked at
40
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/sqlstore/sqlstore.go:3
- Draft comment:
Remove the unused import ofcontext
to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The import of "context" is unnecessary since it's not used in the current code.
2. pkg/sqlstore/sqlstore.go:1
- Draft comment:
The code follows the specified rules correctly. No changes needed. - Reason this comment was not posted:
Confidence changes required:0%
The code does not violate any of the specified rules. It defines an interface for SQLStore without any non-ClickHouse related functions.
Workflow ID: wflow_0LiPByMD5piuSoFL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 c0bf5f5 in 19 seconds
More details
- Looked at
58
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. pkg/sqlstore/config.go:43
- Draft comment:
TheValidate
function currently does not perform any validation. If this is intentional, consider adding a comment explaining why. Otherwise, implement necessary validation logic. - Reason this comment was not posted:
Confidence changes required:50%
The Validate function is currently a no-op, which might be intentional, but it's worth noting that it doesn't perform any validation.
Workflow ID: wflow_oubdkaeqtWQ8KTBG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
### Summary Add `sqlstore` package
Summary
Add
sqlstore
packageRelated Issues / PR's
Part of #6782
Important
Introduces
sqlstore
package with SQLite provider, implements factory pattern for configuration and provider management, and refactors instrumentation setup.sqlstore
package withConfig
andSQLStore
interface insqlstore.go
.sqlitesqlstore/provider.go
usingsql.DB
,bun.DB
, andsqlx.DB
.sqlstoretest/provider.go
.ConfigFactory
andProviderFactory
interfaces infactory/config.go
andfactory/provider.go
.NamedMap
for managing named factories infactory/named.go
.factory/config_test.go
,factory/named_test.go
, andfactory/provider_test.go
.instrumentation
package to use new factory pattern inconfig.go
andsdk.go
.instrumentationtest/instrumentation.go
.instrumentation/logger.go
,meter.go
, andtracer.go
as they are refactored intosdk.go
.This description was created by
for c0bf5f5. It will automatically update as commits are pushed.