[Investigation] Investigate conventions and create drafts for documentation #2

Closed fgardt closed 1 year ago

In GitLab by @Slartibartfass2 on Nov 10, 2022, 21:54

Voting

To approve a convention add :heavy_check_mark: as reaction or if you don't approve, comment what you want to be changed.

Conventions to investigate:

[x] Conventional Commits (already decided for this)
- [x] enforcement: pre-commit hook e.g. https://github.com/conventional-changelog/commitlint/tree/master/'Reference to deleted milestone 40'commitlint/config-conventional
[x] branch naming
[x] code style
- [x] flutter: Flutter formatter
- [x] kotlin: Kotlin coding conventions
- [x] swift: ...
[x] Versioning and CHANGELOG
- [ ] Keep a Changelog
- [x] Common Changelog
[x] Markdown formatting
- [ ] Hallmark
- [x] GitLab Lints

In GitLab by @Slartibartfass2 on Nov 11, 2022, 24:22

moved from smart-sensing-library#25

In GitLab by @Slartibartfass2 on Nov 12, 2022, 13:12

mentioned in merge request smart-sensing-library!3

In GitLab by @Slartibartfass2 on Nov 12, 2022, 13:36

Branch naming

The name of a branch consists of: <Prefix>/<Issue ID>-<Issue Name>

Prefix

The prefix is one of the commit types from Conventional Commits, so e.g. a bug fix branch would be named fix/....

Additionaly there's the prefix investigate for investigating e.g. plugins, architectures.

Issue ID

Just the ID of the issue.

Issue Name

Name of the issue in lowercase with whitespaces replaced by hyphens e.g. add-new-feature

Examples

fix/1-fix-big-bug
feat/2-add-new-feature
docs/3-add-description-in-readme
investigate/4-investigate-server-connection-plugin

Workflow

Navigate to the issue for which you want to create a branch.
Click the down arrow next to Create merge request.
Choose if you just want to create a branch or additionally create a MR.
GitLab has automatically created <Issue ID>-<Issue Name> and you just have to add the <prefix>/ part.
Click Create merge request or Create branch according to your previous choice.

In GitLab by @Slartibartfass2 on Nov 12, 2022, 14:33

Code style guide - Dart

We follow the rules of Effective Dart and use the flutter_lints package as recommended set of linter rules. They are enforced by the Dart Formatter and the Dart Analyzer.

Formatting can be automatically done on save when the following is included in the workspace settings (./.vscode/settings.json):

"editor.formatOnSave": true,

All linter rules

Additional rules

Click to expand

### **DO** declare return types. linter rule: [`always_declare_return_types`](https://dart.dev/tools/linter-rules#always_declare_return_types). ### **AVOID** bool literals in conditional expressions. linter rule: [`avoid_bool_literals_in_conditional_expressions`](https://dart.dev/tools/linter-rules#avoid_bool_literals_in_conditional_expressions). ### **AVOID** classes with only static members. linter rule: [`avoid_classes_with_only_static_members`](https://dart.dev/tools/linter-rules#avoid_classes_with_only_static_members). ### **AVOID** returning null for the return types `bool`, `double`, `int` or `num`. linter rule: [`avoid_returning_null`](https://dart.dev/tools/linter-rules#avoid_returning_null). ### **AVOID** returning null for `Future`. linter rule: [`avoid_returning_null_for_future`](https://dart.dev/tools/linter-rules#avoid_returning_null_for_future). ### **AVOID** unused constructor parameters. linter rule: [`avoid_unused_constructor_parameters`](https://dart.dev/tools/linter-rules#avoid_unused_constructor_parameters). ### **AVOID** async functions that return void. linter rule: [`avoid_void_async`](https://dart.dev/tools/linter-rules#avoid_void_async). ### **DO** use cascade invocations. linter rule: [`cascade_invocations`](https://dart.dev/tools/linter-rules#cascade_invocations). ### **DON'T** cast a nullable value to a non-nullable value. linter rule: [`cast_nullable_to_non_nullable`](https://dart.dev/tools/linter-rules#cast_nullable_to_non_nullable). ### **DON'T** invoke asynchronous functions in non-async blocks. linter rule: [`discarded_futures`](https://dart.dev/tools/linter-rules#discarded_futures). ### **DO** await methods that return a `Future` inside of an async method body. linter rule: [`unawaited_futures`](https://dart.dev/tools/linter-rules#unawaited_futures). ### **DON'T** test for conditions composed only by literals, since the value can be inferred at compile time. linter rule: [`literal_only_boolean_expressions`](https://dart.dev/tools/linter-rules#literal_only_boolean_expressions). ### **DO** only throw instances of classes extending either `Exception` or `Error`. linter rule: [`only_throw_errors`](https://dart.dev/tools/linter-rules#only_throw_errors). ### **DON’T** assign new values to parameters of methods or functions. linter rule: [`parameter_assignments`](https://dart.dev/tools/linter-rules#parameter_assignments). ### **PREFER** asserts with message. linter rule: [`prefer_asserts_with_message`](https://dart.dev/tools/linter-rules#prefer_asserts_with_message). ### **DO** use trailing commas for all function calls and declarations. linter rule: [`require_trailing_commas`](https://dart.dev/tools/linter-rules#require_trailing_commas). ### **DO** use trailing commas on all tree structures longer than one line. Without trailing commas, code that builds widget trees or similar types of code tends to be hard to read. Adding the trailing commas allows `dart format` to do its job correctly. Without trailing commas: ```dart Expanded( child: Center(child: Container(width: 64.0, height: 64.0, child: Text("That's a Text"))), ) ``` With trailing commas: ```dart Expanded( child: Center( child: Container( width: 64.0, height: 64.0, child: Text("That's a Text"), ), ), ) ``` ### **DO** test type arguments in operator `==(Object other)`. linter rule: [`test_types_in_equals`](https://dart.dev/tools/linter-rules#test_types_in_equals). ### **AVOID** `throw` in `finally` block. linter rule: [`throw_in_finally`](https://dart.dev/tools/linter-rules#throw_in_finally). ### **AVOID** unnecessary `await` in return. linter rule: [`unnecessary_await_in_return`](https://dart.dev/tools/linter-rules#unnecessary_await_in_return). ### **AVOID** unnecessary `final` for local variables. linter rule: [`unnecessary_final`](https://dart.dev/tools/linter-rules#unnecessary_final). ### **AVOID** unnecessary null aware operator on extension on a nullable type. linter rule: [`unnecessary_null_aware_operator_on_extension_on_nullable`](https://dart.dev/tools/linter-rules#unnecessary_null_aware_operator_on_extension_on_nullable). ### **AVOID** unnecessary null checks. linter rule: [`unnecessary_null_checks`](https://dart.dev/tools/linter-rules#unnecessary_null_checks). ### **AVOID** unnecessary paranthesis. linter rule: [`unnecessary_parenthesis`](https://dart.dev/tools/linter-rules#unnecessary_parenthesis). ### **AVOID** unnecessary raw strings. linter rule: [`unnecessary_raw_strings`](https://dart.dev/tools/linter-rules#unnecessary_raw_strings). ### **AVOID** unnecessary statements. linter rule: [`unnecessary_statements`](https://dart.dev/tools/linter-rules#unnecessary_statements). ### **AVOID** unnecessary `toList()` in spreads. linter rule: [`unnecessary_to_list_in_spreads`](https://dart.dev/tools/linter-rules#unnecessary_to_list_in_spreads). ### **DO** use enums where appropriate. linter rule: [`use_enums`](https://dart.dev/tools/linter-rules#use_enums). ### **PREFER** the use of `intValue.isOdd/isEven` to check for evenness. linter rule: [`use_is_even_rather_than_modulo`](https://dart.dev/tools/linter-rules#use_is_even_rather_than_modulo). ### **PREFER** the use of predefined named constants. linter rule: [`use_named_constants`](https://dart.dev/tools/linter-rules#use_named_constants). ## **DO** use raw string to avoid escapes. linter rule: [`use_raw_strings`](https://dart.dev/tools/linter-rules#use_raw_strings). ### **DO** use super-initializer parameters where possible. linter rule: [`use_super_parameters`](https://dart.dev/tools/linter-rules#use_super_parameters). ### **DO** use the `throwsA` matcher instead of try-catch with `fail()`. linter rule: [`use_test_throws_matchers`](https://dart.dev/tools/linter-rules#use_test_throws_matchers). ### **CONSIDER** exporting publicly visible classes in a single .dart file. For multiple classes that are used together but are in different files, it’s more convenient for users of your library to import a single file rather many at once. If the user wants narrower imports they can always restrict visibility using the `show` keyword. This also helps minimize the publicly visible surface. Example: ```dart /// In src/apple.dart class Apple {} /// In src/orange.dart class Orange {} /// In src/veggies.dart class Potato {} class Tomato {} /// In botanical_fruits.dart export 'src/apple.dart'; export 'src/orange.dart'; // Can also be: export 'src/veggies.dart' hide Potato; export 'src/veggies.dart' show Tomato; /// In squeezer.dart import 'package:plants/botanical_fruits.dart' show Orange; ``` ### **DO** use namespacing when there is ambiguity, e.g. in class names. There are often functions or classes that can collide, e.g. File or Image. If you don't namespace, there will be a compile error. Good: ```dart import 'dart:ui' as ui; import 'package:flutter/material.dart'; ... ui.Image(...) ... ``` Bad: ```dart import 'dart:ui'; import 'package:flutter/material.dart'; ... Image(...) ... // Which Image is this? ``` ### **PREFER** to use show if you only have a few imports from that package. Otherwise, use as. Using `show` can avoid collisions without requiring you to prepend namespaces to types, leading to cleaner code. Good: ```dart import 'package:fancy_style_guide/style.dart' as style; import 'package:flutter/material.dart'; import 'package:math/simple_functions.dart' show Addition, Subtraction; ``` Bad: ```dart import 'package:flutter/material.dart' show Container, Row, Column, Padding, Expanded, ...; ``` ### **PREFER** to use `const` over `final` over `var`. This minimizes the mutability for each member or local variable. ### **PREFER** return `Widget` instead of a specific type of Flutter widget. As our project evolves, we may change the widget type that is returned in a function. For example, we might wrap a widget with a Center. Returning Widget simplifies the refactoring, as the method signature wouldn't have to change. ### **PREFER** storing state in Models instead of state. When storing state that Flutter widgets need to access, prefer to use `ScopedModel` and `ScopedModelDescendant` instead of `StatefulWidget`. A `StatefulWidget` should contain only internal widget state that can be lost without any consequences. Examples of stuff to store in a `ScopedModel`: - User selections - App state - Anything that needs to be shared by widgets Examples of stuff to store in a `StatefulWidget`'s State: - Animation state that doesn't need to be controlled ### **AVOID** mixing named and positional parameters. Instead, `@required` should be used in place of required positional parameters. ### **PREFER** named parameters. In most situations, named parameters are less error prone and easier to read than positional parameters, optional or not. They give users to pass in the parameters in whatever order they please, and make Flutter trees especially clearer.

Investigation note

Click to expand

Linter rules we might want to include as well (but I didn't think they were absolutely necessary): Feel free to check them out. ```yaml - always_put_control_body_on_new_line - avoid_annotating_with_dynamic - avoid_dynamic_calls - avoid_implementing_value_types - avoid_type_to_string - cancel_subscriptions # maybe needed for plugin - close_sinks # maybe needed for plugin - collection_methods_unrelated_type - deprecated_consistency - do_not_use_environment - join_return_with_assignment - leading_newlines_in_multiline_strings - no_adjacent_strings_in_list - prefer_asserts_in_initializer_lists - prefer_constructors_over_static_methods - prefer_final_in_for_each - prefer_foreach - sized_box_shrink_expand # only needed for Demo - use_colored_box # only needed for Demo - use_decorated_box # only needed for Demo ``` I excluded rules which are either not yet in a stable Dart SDK or only necessary when compiling to JS. Linter rules which I didn't find necessary at all: Additional reasons could be incompatibility with Effective Dart or we don't use a certain feature. ```yaml - always_put_required_named_parameters_first - always_specify_types # incompatible with Effective Dart style guide - always_use_package_imports - avoid_escaping_inner_quotes - avoid_final_parameters - prefer_final_parameters - avoid_multiple_declarations_per_line - avoid_redundant_argument_values - avoid_slow_async_io - conditional_uri_does_not_exist - diagnostic_describe_all_properties - eol_at_end_of_file # done by .editorconfig - flutter_style_todos # doesn't work with GitLab - missing_whitespace_between_adjacent_strings - no_default_cases - no_runtimeType_toString - noop_primitive_operations - prefer_double_quotes - prefer_single_quotes - prefer_if_elements_to_conditional_expressions - prefer_int_literals - prefer_null_aware_method_calls - secure_pubspec_urls - sort_constructors_first - sort_unnamed_constructors_first - sort_pub_dependencies - tighten_type_of_initializing_formals - prefer_final_locals # incompatible with unnecessary_final - unsafe_html # we won't use HTML APIs - use_late_for_private_fields_and_variables - use_string_buffers ```

In GitLab by @Slartibartfass2 on Nov 15, 2022, 22:42

marked the checklist item code style as completed

In GitLab by @Slartibartfass2 on Nov 15, 2022, 22:42

marked the checklist item flutter: Style guide for Flutter repo, Flutter formatter as completed

In GitLab by @Slartibartfass2 on Nov 15, 2022, 22:42

marked the checklist item code style as incomplete

In GitLab by @Slartibartfass2 on Nov 16, 2022, 12:38

added 1h 15m of time spent

In GitLab by @Slartibartfass2 on Nov 16, 2022, 12:38

added 4h 30m of time spent

In GitLab by @Slartibartfass2 on Nov 16, 2022, 12:39

added 1h 30m of time spent

In GitLab by @Slartibartfass2 on Nov 16, 2022, 16:04

added 2h 45m of time spent

In GitLab by @Slartibartfass2 on Nov 16, 2022, 17:45

Code style guide - Kotlin

We follow the rules of the Kotlin coding conventions and the Android Kotlin style guide. Both are enforced by ktlint which is in some aspects more strict*. The linter respects the .editorconfig file and can be customized if needed.

In GitLab by @Slartibartfass2 on Nov 16, 2022, 17:57

marked the checklist item kotlin: Kotlin coding conventions as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 18:27

Code style guide - Swift

We follow the rules of the Official raywenderlich.com Swift style guide which is enforced by SwiftLint. The linter can be added to XCode as Build Tool Plugin and there are plugins for AppCode and VSCode.

In GitLab by @Slartibartfass2 on Nov 16, 2022, 18:28

marked the checklist item swift: ... as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 18:28

marked the checklist item code style as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 19:31

marked the checklist item Common Changelog as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 19:31

marked the checklist item Versioning and CHANGELOG as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 19:38

Versioning and Changelog

Our projects Smart Sensing Library and Sensing Plugin adhere to Semantic Versioning (SemVer). The style guide we use for our changelog is Common Changelog.

Example CHANGELOG.md:

# Changelog

## [0.2.1] - 2022-12-24

### Fixed

- Fixed big bug ([#16](https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/smart-sensing-library/-/issues/42)) (Angela Merkel)

## [0.2.0] - 2022-12-12

### Changed

- Changed functionality ([#69](https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/smart-sensing-library/-/issues/69)) (Johnny Sins)

### Added

- Added new feature ([#42](https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/smart-sensing-library/-/issues/42)) (Zaphod Beeblebrox)

## [0.1.0] - 2022-11-16

_Initial release._

[0.2.1]: <https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/smart-sensing-library/-/releases/0_2_1>
[0.2.0]: <https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/smart-sensing-library/-/releases/0_2_0>
[0.1.0]: <https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/smart-sensing-library/-/releases/0_1_0>

Release workflow

Create new issue and MR to release new version
Update CHANGELOG.md
Update the version in pubspec.yaml
Commit, push and merge changes
Create release tag to corresponding commit
Create release in GitLab Releases from release tag
optional: Update git submodules

In GitLab by @Slartibartfass2 on Nov 16, 2022, 20:02

marked the checklist item Markdown formatting as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 20:02

marked the checklist item GitLab Lints as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 20:13

Markdown style guide

GitLab provides some informations how they test there documentation: GitLab documentation testing.

Linters which we can use are markdownlint and Vale.

markdownlint has an extension for Visual Studio Code. Vale has one as well.

This could be an example for documentation linting to improve our documentation.

In GitLab by @Slartibartfass2 on Nov 16, 2022, 20:16

Git hooks

We can add pre-commit and pre-push hooks to reduce the feedback loop using lefthook.

Once configured it can e.g. check in pre-commit that the formatting and linting is correct and in pre-push run unit tests.

In GitLab by @Slartibartfass2 on Nov 16, 2022, 20:16

marked the checklist item enforcement: pre-commit hook e.g. https://github.com/conventional-changelog/commitlint/tree/master/'Reference to deleted milestone 40'commitlint/config-conventional as completed

In GitLab by @Slartibartfass2 on Nov 16, 2022, 20:19

added 2h of time spent

we'll need some good instructions on how to set that up on your own system but looks good

In GitLab by @Slartibartfass2 on Nov 22, 2022, 13:29

Review process

Checkout branch locally
Test added feature/fixed bug
Check if changes comply to style guides
If you want to comment on something, don't click "Add Comment now" in the MR, but "Start a review" (otherwise the assignee receives an email for each comment)
If your review is finished click "Finish review" and eventually add a final comment e.g. "LGTM", "minor improvements required", ...
Add code independent comments as thread, so that the assignee has to answer your comment
DON'T close the threads as assignee, only the reviewers resolve their own threads.
DO answer an every comment, so that the reviewer sees that you saw the comment. This can be as simple as e.g. "Fixed it", "Added it", "Thanks for the advice".

In GitLab by @Slartibartfass2 on Nov 22, 2022, 15:07

mentioned in merge request sensing-plugin!1

In GitLab by @Slartibartfass2 on Nov 22, 2022, 17:28

We can add a bash file which executes the setup of each project. This could look something like this:

Smart Sensing Library:

# Initialize submodule
git submodule update --init
# Run object box runner
...
# Get dependencies
flutter pub get

...

Sensing Plugin:

# Run pigeon code generator
run_pigeon.sh
# Get dependencies
flutter pub get

...

This is dependent on the target OS tho. Windows can't run bash scripts (atleast outside of WSL and flutter inside WSL is not too great).

In GitLab by @Slartibartfass2 on Nov 22, 2022, 18:12

I used the Git Bash for running the pigeon command as bash file. Otherwise we can find another script language which can be executed without much effort.

In GitLab by @Slartibartfass2 on Nov 23, 2022, 24:20

I tested this on the branch tmp/script-test. I added the file setup.ps1.

On windows run:

.\setup.ps1

On linux run:

bash setup.ps1

If there are only platform-independent commands we can simply put every command in there, otherwise we would have to handle it like in https://gitlab.uni-ulm.de/se-anwendungsprojekt-22-23/sensing-plugin/-/commit/88c2b801d1e1b38836ec57feda1e5a0ca512bc84.

Source

In GitLab by @Slartibartfass2 on Nov 23, 2022, 12:03

Development setup

We use different editors/IDEs for each platform/language:

Flutter/Dart: Visual Studio Code
Android/Kotlin: Android Studio
iOS/Swift: XCode

Android Studio and XCode are only required when working on the Sensing Plugin.

Editor/IDE setup

Visual Studio Code

Install recommended plugins listed in .\.vscode\extensions.json:

Go to Extensions Tab
Enter @recommended into the search bar
Install every plugin listed under Workspace Recommendations

Android Studio (Sensing Plugin)

DON'T open .\android\ or .\example\ as workspace directory.

DO open .\example\android\ so that dependencies are recognized.
In the project view (with Android selected) three modules are shown: app, sensing_plugin and Gradle Scripts.
app contains the code for the example and sensing_plugin the code for the plugin.
Enable EditorConfig support by navigating to Settings -> Editor -> Code Style and check Enable EditorConfig support

XCode (Sensing Plugin)

DON'T open Xcode via right-clicking on the .\example\ios\ folder and click "Open in Xcode".

DO open Xcode directly.
Click "Open..." or "Cmd + O".
Open the .\example\ios\Runner.xcworkspace.
All things should be initialized automatically.

Repository setup

We use scripts to execute shell commands for the project setup or to generate code:

On Linux/MacOS run: bash <file name>
On Windows run: .\<file name> Note: They are powershell scripts but work as bash script as well.

For the setup on the Sensing Plugin and Smart Sensing Library run the setup.ps1 script.

Sensing Plugin

We use the Pigeon package to generate API code for each platform. If you edit [Insert pigeon file here e.g. .\pigeons\messages.dart] run the pigeon.ps1 script.

Smart Sensing Library

We use the ObjectBox package as database system. If you edit [Inset object box file here] run the objectbox.ps1 script.

In GitLab by @Slartibartfass2 on Nov 25, 2022, 15:53

marked this issue as related to #15

In GitLab by @Merseleo on Nov 25, 2022, 16:00

Do the Dart plugin show me a warning, if I "violate" one of these rules?

In GitLab by @Slartibartfass2 on Nov 25, 2022, 16:08

mentioned in merge request sensing-plugin!2

In GitLab by @Merseleo on Nov 25, 2022, 16:11

Please link the Tutorial how to install SwiftLint in Xcode somewhere.

In GitLab by @Slartibartfass2 on Nov 25, 2022, 16:13

Yes, in VSCode the violation will be underlinded and shown under "Problems".

In GitLab by @Merseleo on Nov 25, 2022, 16:17

But it is not very complicated to prove the style guides every time manually? Isn't this the job of the linter?

In GitLab by @Slartibartfass2 on Nov 25, 2022, 16:29

Mainly yes, I rather meant style guides which aren't enforced by the linter e.g. design patters or basic "good" design ideas.

In GitLab by @Slartibartfass2 on Nov 25, 2022, 16:31

This will be added when adding the linter which will be probably at some point in se-anwendungsprojekt-22-23&7.

In GitLab by @Slartibartfass2 on Nov 28, 2022, 18:29

mentioned in issue smart-sensing-library#31

In GitLab by @Merseleo on Nov 28, 2022, 21:49

Ok, perfect

In GitLab by @Merseleo on Nov 28, 2022, 21:49

Make sense and seems a very good idea.

In GitLab by @Slartibartfass2 on Nov 29, 2022, 24:15

Project structure

Dart provides a guide on how to design a good project structure which we should follow. The following graph should be a basic design of how this would apply on our projects.

graph TD
    A[Root] --> B[lib]
    B --> C[src]
    B --> BA(sensing_plugin.dart)
    C --> CA(sensor_manager.dart)
    C --> CB[generated]
    CB --> CBA(api.dart)
    A --> D[test]
    D --> DA(sensor_manager_test.dart)
    A --> E[tool]
    E --> EA(setup.ps1)
    E --> EB(run_pigeon.ps1)
    A --> F[pigeons]
    F --> FA(api.dart)

Note there's also example as subdirectory of root which I didn't include.

@Shiroen95 Do we need to take the ObjectBox plugin into account when designing the project structure? Which files are generated where and how are they organized?

In GitLab by @Shiroen95 on Nov 29, 2022, 14:29

The following is from the ObjectBox wiki:

Run flutter pub run build_runner build to generate the binding code required to use ObjectBox.

ObjectBox generator will look for all @Entity annotations in your lib folder and create a single database definition lib/objectbox-model.json and supporting code in lib/objectbox.g.dart

Add objectbox.g.dart to your version control ignore list (e.g. .gitignore), otherwise build_runner will complain about it being changed each time you pull a change.

So the files kept are all in the /lib folder. So the only thing we need to make sure is that we keep our entities somewhere in the /lib/* folder.

In GitLab by @Slartibartfass2 on Jan 22, 2023, 20:54

mentioned in issue sensing-plugin#24

In GitLab by @Merseleo on Mar 24, 2023, 13:51

mentioned in commit c372c46e267dce840c2c78282c81f0a12123ea04

uulm-smart-sensing / documentation