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

fix: notify queries when executing group statements with result #5006

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vitorhugods
Copy link
Contributor

@vitorhugods vitorhugods commented Feb 4, 2024

Fixes:


Proposed solution

By modifying the QueryGenerator to make sure all child classes have access to tablesUpdated(), and centralising the query notification into the executeBlock function, make sure the following is added to the generated code of grouped statements.

} .also {
    notifyQueries(queryId) { emit ->
      // emit table names
    }
}

I am not completely sure of this approach and I'd love some feedback before I dedicate more time into it.

To be solved yet

Annoying space before .also {. I'll learn a bit more of Kotlinpoet :)

Questions

  • What's the importance of the Query ID? I noticed in some tests it is hardcoded, in some it figures it out from the compiler. I can modify the notifyQueriesBlock() to accept the QueryId and make it match one of the statements.

Remark

I am not sure about the executeBlock. I find its control flow a bit hard to follow and I'd love to simplify a bit and maybe link the QueryGenerator's implementations more closely to implement it depending on what the query looks like, instead of having too many ifs and elses.

@vitorhugods vitorhugods changed the title fix: W.I.P. make sure notify is called when executing group statements with result fix: notify queries when executing group statements with result Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant