Skip to content

Add dql_strict_mode=false to all quickstarts #88

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

Closed
wants to merge 7 commits into from

Conversation

okdistribute
Copy link
Contributor

Fixes MAR-132

Comment on lines 152 to 154
_ditto!.store.execute("ALTER SYSTEM SET DQL_STRICT_MODE = false").then((_) {
setState(() => _ditto!.startSync());
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be set whenever sync is stopped and started again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Slop trying to do it's best because I said "set this before startSync is called"

@@ -313,6 +313,7 @@ private void StartSync()
{
try
{
ditto.Store.Execute("ALTER SYSTEM SET DQL_STRICT_MODE = false");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an async method that would need to be awaited. And the name is ExecuteAsync

@@ -250,6 +250,7 @@ public DittoStoreObserver ObserveTasksCollection(Func<IList<ToDoTask>, Task> han
/// </summary>
public void StartSync()
{
ditto.Store.Execute("ALTER SYSTEM SET DQL_STRICT_MODE = false");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an async method that would need to be awaited. And the name is ExecuteAsync

@@ -246,6 +246,7 @@ public DittoStoreObserver ObserveTasksCollection(Func<IList<ToDoTask?>, Task> ha
/// </summary>
public void StartSync()
{
ditto.Store.Execute("ALTER SYSTEM SET DQL_STRICT_MODE = false");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an async method that would need to be awaited. And the name is ExecuteAsync

Comment on lines 58 to 60
Task {
try await dittoStore.execute(query: "ALTER SYSTEM SET DQL_STRICT_MODE = false")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken a closer look: I suspect the intention here is to bubble up any error happening within the Task scope to the caller of startSync(). Correct?

Task is performed asynchronously and errors are swallowed, i.e. go unnoticed. So if the query fails for some reason, sync will still be started and no error will be presented or anything.

If my assumption is correct, then you'd have to rewrite the code path and make it account for async execution and error handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding await requires this to be async?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me with this one? I don't fully understand Swift async (and I know it's a sore spot in our SDK)

Copy link
Contributor

@konstantinbe konstantinbe Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Is there a reason why this is executed as part of start sync button? It looks like it needs to be executed once, ideally at initialization time. Here is the simplest solution: #91. Adding proper error handling would require another 1 - 2 pages of SwiftUI code and would need to be ported to all the others — not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason only being AI slop ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that counts as an excuse tbh.

Copy link
Contributor Author

@okdistribute okdistribute Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an excuse, just a reality of time constraints. I can close the PR and prefer others to finish this out

@okdistribute
Copy link
Contributor Author

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.

5 participants