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

refactor(sqlite): move migrations logic to sqlite common module #1325

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Nov 2, 2022

As an intermediate step, I decided to do a minor cleanup before we take another decision about what to do with the SQLite migrations. I moved the logic to the waku/common module where SQLite database code was moved previously.

  • Move migrations to common/sqlite module since this code is agnostic to any implementation.
  • Update tests accordingly

@LNSD LNSD requested review from rymnc and jm-clius November 2, 2022 09:33
@LNSD LNSD self-assigned this Nov 2, 2022
@LNSD LNSD force-pushed the refactor-sqlite-migrations branch from 92aac88 to 194f51c Compare November 2, 2022 09:36
@status-im-auto
Copy link
Collaborator

status-im-auto commented Nov 2, 2022

Jenkins Builds

Click to see older builds (2)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 92aac88 #1 2022-11-02 09:51:49 ~17 min macos 📦bin
92aac88 #1 2022-11-02 09:57:10 ~22 min linux 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
194f51c #2 2022-11-02 09:56:24 ~20 min linux 📄log
✔️ 194f51c #2 2022-11-02 09:58:39 ~22 min macos 📦bin
✔️ 475839c #3 2022-11-02 10:30:18 ~16 min macos 📦bin
✔️ 475839c #3 2022-11-02 10:31:49 ~17 min linux 📦bin

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM!

waku/common/sqlite/database.nim Show resolved Hide resolved
waku/common/sqlite/migrations.nim Outdated Show resolved Hide resolved
@LNSD LNSD force-pushed the refactor-sqlite-migrations branch from 194f51c to 475839c Compare November 2, 2022 10:13
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,334 +1,7 @@
{.push raises: [Defect].}
{.push raises: [].}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for starting to add these. Can you see any (good) reason for us to rather use:

when (NimMajor, NimMinor) < (1, 4):
  {.push raises: [Defect].}
else:
  {.push raises: [].}

?

Perhaps if someone compiles nwaku into a system with an older nim version (unlikely and should probably not be allowed) or (perhaps more likely) that the nimbus-build-system gets downgraded due to some unforeseen issues?

Copy link
Contributor Author

@LNSD LNSD Nov 2, 2022

Choose a reason for hiding this comment

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

I removed the Defect because the compiler added extra noise because of that pragma. I did it as an experiment, and I forgot to restore it.

Perhaps if someone compiles nwaku into a system with an older nim version (unlikely and should probably not be allowed) or (perhaps more likely) that the nimbus-build-system gets downgraded due to some unforeseen issues?

Absolutely! Let's do this in a unique PR where we change all the occurrences. WDYT? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, absolutely. This can be merged as is IMO. For me there is the more general questions on why we would want to keep the modules compatible with Nim <1.4, but that is a separate discussion and cost is low in any case.

@LNSD LNSD merged commit 1d3943f into master Nov 2, 2022
@LNSD LNSD deleted the refactor-sqlite-migrations branch November 2, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants