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.

Sign up for GitHub

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

Jump to bottom

Support directory for windows #378

Closed
wants to merge 2 commits into from
Closed

Support directory for windows #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b0e9e8
Support directory for windows
hortigado May 7, 2024
4605f32
Update sqlite_provider.dart
hortigado May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions packages/brick_sqlite/lib/src/sqlite_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import 'package:brick_sqlite/src/models/sqlite_model.dart';
import 'package:brick_sqlite/src/sqlite_model_dictionary.dart';
import 'package:logging/logging.dart';
import 'package:meta/meta.dart';
import 'package:sqflite_common/sqlite_api.dart';
import 'package:path/path.dart';
import 'package:path_provider/path_provider.dart';
import 'package:sqflite_common/sqflite.dart';
import 'package:sqflite_common/utils/utils.dart' as sqlite_utils;
import 'package:synchronized/synchronized.dart';

Expand Down Expand Up @@ -136,8 +138,17 @@ class SqliteProvider implements Provider<SqliteModel> {

/// Access the latest instantiation of the database [safely](https://github.com/tekartik/sqflite/blob/master/sqflite/doc/opening_db.md#prevent-database-locked-issue).
@protected
Future<Database> getDb() {
_openDb ??= databaseFactory.openDatabase(dbName);
Future<Database> getDb() async {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is a little misleading to traditional async usage. It requires that the return value is an unfulfilled Future. This protects openDatabase from being called more than once. More on this soon, but I wanted to give you a quick thesis: this function shouldn't have any awaits.

Copy link
Author

Choose a reason for hiding this comment

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

It's true that it doesn't cover the cachemanager. The truth is, I think that in that case the best thing would be to separate it into an auxiliary function. I was also not aware of the problem with sqlite with async.
I didn't analyze the code much because it was a quick fix that I had placed for testing.
What should I do now to make the changes or would you do them yourself? I don't really understand how this world of PR works haha.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can do them myself, although you wouldn't get the credit for the code. Is that alright with you if I make a PR with applying these suggestions to the documentation?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, better than you, I still have to learn a little more to make a more elaborate PR. Greetings

final String dbPath;
if (Platform.isWindows) {
final Directory directory = await getApplicationSupportDirectory();
dbPath = directory.path;
} else {
dbPath = await getDatabasesPath();
}
final String path = join(dbPath, dbName);
_openDb ??= openDatabase(path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

getDb is invoked multiple times throughout sqliteProvider. It needs to execute quickly, if at all, which is why there's memoization with ??=.


return _openDb!;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/brick_sqlite/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ dependencies:
meta: ">=1.3.0 <2.0.0"
sqflite_common: ">=2.0.0 <3.0.0"
synchronized: ^3.0.0

path_provider: ^2.1.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

brick_sqlite doesn't have a flutter dependency, which makes it very nimble and suitable to all sorts of environments (especially FFI). This was a big part of Brick v2. path_provider would reintroduce a Flutter dependency.

path: ^1.9.0
dev_dependencies:
test: ^1.16.5
mockito: ^5.0.0
Expand Down