-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat(neon_framework): add neon_storage library #2487
Conversation
40478d6
to
15dea3e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2487 +/- ##
==========================================
+ Coverage 28.52% 28.56% +0.03%
==========================================
Files 355 359 +4
Lines 136003 136078 +75
==========================================
+ Hits 38794 38865 +71
- Misses 97209 97213 +4
*This pull request uses carry forward flags. Click here to find out more.
|
15dea3e
to
43e5390
Compare
I rebased it onto main and dropped the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few suggestions.
packages/neon_framework/packages/neon_storage/analysis_options.yaml
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/lib/neon_sqlite.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/lib/src/sqlite/multi_table_database.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/lib/src/sqlite/multi_table_database.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/lib/src/sqlite/multi_table_database.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
43e5390
to
5d9397f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add the verifyNever()
after each verify()
(except for cases with no arguments)?
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
packages/neon_framework/packages/neon_storage/test/sqlite/multi_table_database_test.dart
Outdated
Show resolved
Hide resolved
The teardown will now check that all calls are covered by a verify. I see this as redundant. |
I missed that 👍 |
Signed-off-by: Nikolas Rimikis <[email protected]>
5d9397f
to
db04e40
Compare
We talked about it a lot, and it's finally here.
The
neon_storage
package does not depend on any platform plugin, thus it's 100% dart only. This would also allow our repositories to not depend on flutter for testing.Long term, I'd like the custom storage magic to be removed and every storage component to use a custom table.
I already have a branch for the
account_repository
that I can share if that helps you understand this PR.This also moves the entire platform selection logic into the package so it can be used separately.