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

feat(database): add initial firebase database #4

Merged
merged 5 commits into from
Aug 4, 2023
Merged

feat(database): add initial firebase database #4

merged 5 commits into from
Aug 4, 2023

Conversation

daeyeon
Copy link
Contributor

@daeyeon daeyeon commented Jul 24, 2023

See this link for the message channel protocol.

Signed-off-by: Daeyeon Jeong [email protected]

@daeyeon
Copy link
Contributor Author

daeyeon commented Jul 24, 2023

cc @JSUYA

@daeyeon daeyeon requested a review from JSUYA July 30, 2023 23:10
Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

I left a few reviews. Please check.

+)
Most TRACE() rules are ambiguous. There are a tag, an error or warning, simple call checks.
At least I think it can be cleaned up just by adding the appropriate tags.

packages/firebase_database/README.md Outdated Show resolved Hide resolved
packages/firebase_database/analysis_options.yaml Outdated Show resolved Hide resolved
packages/firebase_database/example/pubspec.yaml Outdated Show resolved Hide resolved
packages/firebase_database/pubspec.yaml Outdated Show resolved Hide resolved
packages/firebase_database/example/lib/main.dart Outdated Show resolved Hide resolved
.value_or(false));

if (GetOptionalValue<int32_t>(args, Constants::kDatabaseCacheSizeBytes)) {
TRACE(DATABASE, "[!] cacheSizeBytes isn't supported.");
Copy link
Member

Choose a reason for hiding this comment

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

There are already other PR prefixes using [!] but I think it would be better if these were words that meant something.
It is recommended to replace it with [WARN] or [WARNING].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most TRACE() rules are ambiguous. There are a tag, an error or warning, simple call checks. At least I think it can be cleaned up just by adding the appropriate tags.

Make sense. However, since formatting the logs is trivial, why don't we apply the rules to all plugins in another PR? By the way, please note that all the TRACEs are removed in the release build and will never be shown to users, even in debug mode. This already resolves our main concerns about verbose logs for users.

@daeyeon
Copy link
Contributor Author

daeyeon commented Aug 2, 2023

Applied suggestions from code review. @JSUYA PTAL when you have a moment.

Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

This is result of dart analyze.
Please run and check dart analyze

Analyzing firebase_database...

   info - example/integration_test/firebase_database/database_e2e.dart:8:8 - Can't use a relative path to import a library in 'lib'. Try fixing the relative path or changing the import to a 'package:' import. - avoid_relative_lib_imports
   info - example/integration_test/firebase_database/database_e2e.dart:63:10 - Unnecessary braces in a string interpolation. Try removing the braces. - unnecessary_brace_in_string_interps
   info - example/integration_test/firebase_database/database_e2e.dart:70:10 - Unnecessary braces in a string interpolation. Try removing the braces. - unnecessary_brace_in_string_interps
   info - example/integration_test/firebase_database/database_e2e.dart:75:10 - Unnecessary braces in a string interpolation. Try removing the braces. - unnecessary_brace_in_string_interps
   info - example/integration_test/firebase_database/database_reference_e2e.dart:141:46 - Missing a required trailing comma. - require_trailing_commas
   info - example/integration_test/firebase_database/extra_e2e.dart:22:7 - Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await' or wrapping the expression un 'unawaited'. - unawaited_futures
   info - example/integration_test/firebase_database/extra_e2e.dart:37:7 - Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await' or wrapping the expression un 'unawaited'. - unawaited_futures
   info - example/integration_test/firebase_database/query_e2e.dart:81:55 - Missing a required trailing comma. - require_trailing_commas
   info - example/integration_test/firebase_database/query_e2e.dart:155:54 - Missing a required trailing comma. - require_trailing_commas
   info - example/lib/main.dart:128:50 - 'headline4' is deprecated and shouldn't be used. Use headlineMedium instead. This feature was deprecated after v3.1.0-0.0.pre. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - pubspec.yaml:25:3 - Unsorted dependencies. Try sorting the dependencies. - sort_pub_dependencies

11 issues found.

@daeyeon
Copy link
Contributor Author

daeyeon commented Aug 3, 2023

@JSUYA Regarding the dart analyze result, we don't plan to improve the tests and the example ourselves.

@JSUYA
Copy link
Member

JSUYA commented Aug 3, 2023

@JSUYA Regarding the dart analyze result, we don't plan to improve the tests and the example ourselves.

It is not a matter of improving the example and test code.
Since this project is based on firebase_database 10.0.9. analyze_options and more files should be changed.
(We have to make sure that no errors are found in dart analyze.)

This is firebase/firebase_database/example/analysis_options.yaml (ver 10.0.9)

include: ../../../../analysis_options.yaml
linter:
  rules:
    avoid_print: false
    depend_on_referenced_packages: false
    library_private_types_in_public_api: false

@daeyeon
Copy link
Contributor Author

daeyeon commented Aug 3, 2023

@JSUYA As you may know, the integration_test sources have been merged into the example directory, so we can't simply get the errors passed using the lint option above. And, as discussed already, the integration_test sources will be moved to a different path in the future, making it easier to fix the lint errors. So, it dosen't seem worth improving the test sources or any options in this PR right now. What do you think?

@JSUYA
Copy link
Member

JSUYA commented Aug 3, 2023

~/dev/os/f-project/flutterfire/packages/firebase_database (dev/db) 17:27:27 $ git diff
diff --git a/all_lint_rules.yaml b/all_lint_rules.yaml
index 0375a83..7f933bb 100644
--- a/all_lint_rules.yaml
+++ b/all_lint_rules.yaml
@@ -193,4 +193,4 @@ linter:
     - use_test_throws_matchers
     - use_to_and_as_if_applicable
     - valid_regexps
-    - void_checks
+    - void_checks
\ No newline at end of file
diff --git a/analysis_options.yaml b/analysis_options.yaml
index 2156d38..79a2ff5 100644
--- a/analysis_options.yaml
+++ b/analysis_options.yaml
@@ -74,6 +74,10 @@ linter:
     # and `@required Widget child` last.
     always_put_required_named_parameters_first: false

+    # `as` is not that bad (especially with the upcoming non-nullable types).
+    # Explicit exceptions is better than implicit exceptions.
+    avoid_as: false
+
     # This project doesn't use Flutter-style todos
     flutter_style_todos: false

diff --git a/packages/firebase_database/example/analysis_options.yaml b/packages/firebase_database/example/analysis_options.yaml
index a1fbb31..6b3fb09 100644
--- a/packages/firebase_database/example/analysis_options.yaml
+++ b/packages/firebase_database/example/analysis_options.yaml
@@ -2,4 +2,9 @@
 # Use of this source code is governed by a BSD-style license that can be
 # in the LICENSE file.

-include: ../analysis_options.yaml
+include: ../../../analysis_options.yaml
+linter:
+  rules:
+    avoid_print: false
+    depend_on_referenced_packages: false
+    library_private_types_in_public_api: false
\ No newline at end of file

@JSUYA
Copy link
Member

JSUYA commented Aug 3, 2023

~/dev/os/f-project/flutterfire/packages/firebase_database (dev/db) 17:27:32 $ dart analyze
Analyzing firebase_database...         0.7s

   info • example/integration_test/firebase_database/database_e2e.dart:8:8 • Can't use a relative path to import a library in 'lib'. Try fixing the relative path
          or changing the import to a 'package:' import. • avoid_relative_lib_imports
   info • example/integration_test/firebase_database/database_e2e.dart:63:10 • Unnecessary braces in a string interpolation. Try removing the braces. •
          unnecessary_brace_in_string_interps
   info • example/integration_test/firebase_database/database_e2e.dart:70:10 • Unnecessary braces in a string interpolation. Try removing the braces. •
          unnecessary_brace_in_string_interps
   info • example/integration_test/firebase_database/database_e2e.dart:75:10 • Unnecessary braces in a string interpolation. Try removing the braces. •
          unnecessary_brace_in_string_interps
   info • example/integration_test/firebase_database/database_reference_e2e.dart:141:46 • Missing a required trailing comma. • require_trailing_commas
   info • example/integration_test/firebase_database/extra_e2e.dart:22:7 • Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await'
          or wrapping the expression un 'unawaited'. • unawaited_futures
   info • example/integration_test/firebase_database/extra_e2e.dart:37:7 • Missing an 'await' for the 'Future' computed by this expression. Try adding an 'await'
          or wrapping the expression un 'unawaited'. • unawaited_futures
   info • example/integration_test/firebase_database/query_e2e.dart:81:55 • Missing a required trailing comma. • require_trailing_commas
   info • example/integration_test/firebase_database/query_e2e.dart:155:54 • Missing a required trailing comma. • require_trailing_commas
   info • example/lib/main.dart:225:59 • The expression has no effect and can be removed. Try removing the expression. • noop_primitive_operations

10 issues found.

@daeyeon
Copy link
Contributor Author

daeyeon commented Aug 3, 2023

10 issues found.

@hs0225 hs0225 merged commit c125156 into main Aug 4, 2023
3 checks passed
@daeyeon daeyeon deleted the dev/db branch August 4, 2023 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants