Looks good, can we maybe move get_database_version() closer to where it's used though (it's like 200 lines above the only usage)? Feel free to leave as is if you had a reason to place it where you did.

This revision is now accepted and ready to land.Oct 19 2022, 12:31 PM

In D5375#159611, @atul wrote:

Looks good, can we maybe move get_database_version() closer to where it's used though (it's like 200 lines above the only usage)? Feel free to leave as is if you had a reason to place it where you did.

Well I thought that SQLiteQueryExecutor.cpp is divided into logical parts

operations on the database using raw SQL
operations connected to directly database file
encryption code
migrations
operation on the database using ORM

so If I am wrong and it'll be better to place it lower it's still okay to me, I was just trying to fit this in the best place

In the future there will be a need to get the database version in different places, that being said extracting logic to separate function.

What do you mean by "other" places? If you think about calling this function from different places inside SQliteQueryExecutor then this differential is fine and ready to be accepted. But if you want to call this function somewhere in C++ outside of SQliteQueryExecutor, then we have a few problems:

This function is not defined in SQLiteQueryExecutor.h neither as a class method nor free function. So we are not able to call it outside of SQLIteQueryExecutor
If we want to call this function outside of SQLiteQueryExecutor.h should it be free function or method? If method then we also need to introduce it as virtual in DatabaseQueryExecutor abstract class

I need your answer regarding "other places" in the code we want to call this function from and your thoughts on 1 and 2 if you want to call this function outside of SQLiteQueryExecutor.

This revision now requires changes to proceed.Oct 20 2022, 12:25 PM

marcin mentioned this in D5376: [native/sqlite] introduce function to set database version.Oct 20 2022, 12:27 PM

In D5375#160617, @marcin wrote:

In the future there will be a need to get the database version in different places, that being said extracting logic to separate function.

What do you mean by "other" places? If you think about calling this function from different places inside SQliteQueryExecutor then this differential is fine and ready to be accepted. But if you want to call this function somewhere in C++ outside of SQliteQueryExecutor, then we have a few problems:

This function is not defined in SQLiteQueryExecutor.h neither as a class method nor free function. So we are not able to call it outside of SQLIteQueryExecutor

If we want to call this function outside of SQLiteQueryExecutor.h should it be free function or method? If method then we also need to introduce it as virtual in DatabaseQueryExecutor abstract class

I need your answer regarding "other places" in the code we want to call this function from and your thoughts on 1 and 2 if you want to call this function outside of SQLiteQueryExecutor.

I think it's obvious that it'll be called only inside SQLiteQueryExecutor because I didn't add this to SQLiteQueryExecutor.h, different places (not "other" places) means multiple lines inside SQLiteQueryExecutor - sorry for confusing you if the word is not proper for this case.

marcin accepted this revision.Oct 21 2022, 1:26 AM

marcin added a reviewer: tomek.

tomek accepted this revision.Oct 21 2022, 2:26 AM

This revision is now accepted and ready to land.Oct 21 2022, 2:26 AM

Closed by commit rCOMM1824b7d8ba49: [native/sqlite] introduce function to get database version (authored by kamil). · Explain WhyOct 25 2022, 12:46 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM1824b7d8ba49: [native/sqlite] introduce function to get database version.