Skip to content
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

Introduce Query builder #17

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Introduce Query builder #17

wants to merge 2 commits into from

Conversation

yhabteab
Copy link
Member

refs #6

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label May 27, 2024
@yhabteab yhabteab requested a review from oxzi May 27, 2024 08:03
@oxzi oxzi mentioned this pull request May 27, 2024
database/query_builder.go Outdated Show resolved Hide resolved
database/query_builder.go Outdated Show resolved Hide resolved
database/query_builder_test.go Outdated Show resolved Hide resolved
database/query_builder.go Show resolved Hide resolved
database/query_builder.go Show resolved Hide resolved
database/query_builder.go Outdated Show resolved Hide resolved
database/query_builder.go Outdated Show resolved Hide resolved
database/query_builder.go Outdated Show resolved Hide resolved
database/query_builder.go Outdated Show resolved Hide resolved
database/query_builder.go Outdated Show resolved Hide resolved
@lippserd
Copy link
Member

I would find it clearer if the API did not require the query builder to be created again and again. It would also be more flexible if the different statement types were separate structs.

@julianbrost
Copy link
Contributor

If we're starting to discuss the overall API design this PR won't do it to get Icinga/icinga-notifications#114 merged any time soon. So either we merge this as an experimental API (i.e. might fundamentally change) or we need another PR that simply exposes a BuildColumns() function again so that this can be used by Icinga/icinga-notifications#114 in the meantime.

@lippserd
Copy link
Member

If we're starting to discuss the overall API design this PR won't do it to get Icinga/icinga-notifications#114 merged any time soon. So either we merge this as an experimental API (i.e. might fundamentally change) or we need another PR that simply exposes a BuildColumns() function again so that this can be used by Icinga/icinga-notifications#114 in the meantime.

I see. Then let’s just merge it.

@julianbrost
Copy link
Contributor

or we need another PR that simply exposes a BuildColumns() function again so that this can be used by Icinga/icinga-notifications#114 in the meantime

I've asked for this to be done nonetheless as I find the current interface this PR adds more confusing than it needs to be in regards to what it accepts in any parameters. And even if it's kept that way, the current implementation has room for improvement by reducing the amount of use of the reflect package. All in all, more than what should be necessary to finally get Icinga Notifications away from importing Icinga DB as a package.

@yhabteab yhabteab marked this pull request as draft July 19, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants