Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

Migrated to Hive #16

Merged
32 commits merged into from
Nov 29, 2023
Merged

Migrated to Hive #16

32 commits merged into from
Nov 29, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 20, 2023

Purpose

Migrating from Shared Preferences to Hive.
https://concordium.atlassian.net/browse/CBW-1474

Changes

  • Migrated from Shared Preferences to Hive
  • Renamed from <Prefix>Service to <Prefix>Provider for StorageProvider to follow the naming conventions of the Bloc architecture.
    https://bloclibrary.dev/#/architecture?id=data-provider
  • Renamed pipeline to just ci.yml since test step has been added.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.

.github/workflows/ci.yml Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
lib/providers/storage.dart Outdated Show resolved Hide resolved
lib/providers/storage.dart Outdated Show resolved Hide resolved
lib/entities/accepted_terms_and_conditions.dart Outdated Show resolved Hide resolved
lib/state/services.dart Show resolved Hide resolved
lib/state/terms_and_conditions.dart Outdated Show resolved Hide resolved
lib/state/terms_and_conditions.dart Outdated Show resolved Hide resolved
lib/state/terms_and_conditions.dart Outdated Show resolved Hide resolved
test/providers/storage_test.dart Outdated Show resolved Hide resolved
@orhoj orhoj assigned ghost Nov 22, 2023
lib/providers/storage.dart Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
lib/state/terms_and_conditions.dart Outdated Show resolved Hide resolved
lib/entities/accepted_terms_and_conditions.dart Outdated Show resolved Hide resolved
lib/providers/storage.dart Outdated Show resolved Hide resolved
lib/providers/storage.dart Outdated Show resolved Hide resolved
lib/state/services.dart Show resolved Hide resolved
lib/state/terms_and_conditions.dart Outdated Show resolved Hide resolved
bisgardo and others added 13 commits November 22, 2023 12:42
The current widget 'App' uses two nested `FutureBuilder`s in a wrong way: The provided future should not be constructed inside the build method. As the [documentation](https://api.flutter.dev/flutter/widgets/FutureBuilder-class.html#managing-the-future) puts it:

> The future must have been obtained earlier, e.g. during State.initState, State.didUpdateWidget, or State.didChangeDependencies. It must not be created during the State.build or StatelessWidget.build method call when constructing the FutureBuilder. If the future is created at the same time as the FutureBuilder, then every time the FutureBuilder's parent is rebuilt, the asynchronous task will be restarted.

This behavior has been observed during development as flickering and crashing due to duplicate network activation.

With this change the two `FutureBuilder`s now live in their own separate stateful widgets where the futures are constructed by their respective `initState` methods.

This change has the furtunate side effect of making the original widget much simpler and easier to understand.
@ghost ghost requested review from orhoj and bisgardo November 25, 2023 19:52
lib/providers/storage.dart Show resolved Hide resolved
test/providers/storage_test.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
@ghost ghost requested a review from orhoj November 28, 2023 08:42
lib/main.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
@ghost ghost requested a review from orhoj November 29, 2023 09:21
@ghost ghost merged commit 17c6730 into main Nov 29, 2023
2 checks passed
@ghost ghost deleted the cbw-1474/migrate-to-hive branch November 29, 2023 09:32
@bisgardo
Copy link
Contributor

@schwartz-concordium I think it's unfortunate that this was merged without squashing. Please take a look at https://github.com/Concordium/concordium-wallet/commits/main and try to make sense of it. What commits belong to which PRs? It may be obvious in this case by looking at the author, but imagine that all PRs were like this. Then you'd have a commit history that's very far from being as clear as it could be, without any gains.

But also, many of the commits tell very little about why the changes were made. And many contain issues that were known and fixed before merging. This makes tools like git blame much less effective.

My strong personal view is that the commit history is the most useful when every commit is a progressive improvement (complete unit of work if you will) and gives a clear explanation of why it's done. This can certainly be done properly without squashing but it's much more work (i.e. rebasing) and is in my opinion done better just as multiple PRs.

I'm sorry for ranting but this matters a lot to me. I'll be very happy to do this discussion with you and the team offline.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants