0.2 Release #16
0.2 Release #16
Conversation
|
Dumb question: the Flutter wiki (https://github.com/flutter/flutter/wiki/Desktop-shells) makes it sound like desktop support is, well, a dumpster fire. Why was the mobile app example replaced with a desktop example? |
There was a problem hiding this comment.
This is hard to review because everything shows as new.
Consider using git mv source target command in the future, if your IDE doesn't do it on your own (VSCode doesn't). It works with whole directories as well.
Please consider the package layout conventions I've posted and ping me for another review afterwards
| 3. Finally run `dart test/test.dart` to start the demo script. | ||
| Note that, as fairly recent language features are used, the minimal required Dart version is 2.2.2. | ||
| 1. Install [objectbox-c](https://github.com/objectbox/objectbox-c) system-wide: `curl -s https://github.com/raw/objectbox/objectbox-c/master/download.sh > /tmp/download.sh && bash /tmp/download.sh 0.7 && rm /tmp/download.sh` (answer Y when it asks about installing to /usr/lib). | ||
| 2. Back in this repository, run `pub get` in each of the following directories: `objectbox`, `objectbox_model_generator`, `objectbox_test`. If you just want to try out the tests, running it in `objectbox_test` is enough. |
There was a problem hiding this comment.
The new branch doesn't follow conventions https://dart.dev/tools/pub/package-layout or rather it creates three packages.
What is the reason it must be split in such a way instead having a single package?
- lib instead of objectbox
- bin/binding-generator instead of objectbox_model_generator
- test instead of objectbox_test
There was a problem hiding this comment.
For this, I generally stuck to how Dart's json_serializable organizes its files. It uses source_gen as well (just like objectbox_model_generator), so it seemed like quite a good reference for some first orientation. Also, it subdivides the repository into three subprojects as well, each with its own pubspec.yaml: example (our objectbox_test), json_annotation (our objectbox) and json_serializable (our objectbox_model_generator).
Unfortunately, source_gen and build_runner are rather poorly documented for the use case of generating source code for annotations, so it's hard to find good resources on how to better organize the directory structure. Apart from that, I also think that the model generator needs to have its own Dart package in order for build.yaml to work properly.
There was a problem hiding this comment.
Core packages are not always a best source of organization since they use to have long history, before most conventions even emerged.
The page I've linked (https://dart.dev/tools/pub/package-layout) is an official recommendation how a package should be structured and as such it would be strongly preferable to follow there are serious reasons we need to diverge. If you say building the generator is (I don't know enough of dart to help you there at the moment), then let's keep the generator as a separate project for now but at least leave the rest as a single one. What do you think?
There was a problem hiding this comment.
Great, I definitely agree; I'll also keep the generator as a separate package for now. Please check out my last commit, I reorganized the file structure similar to how you suggested.
|
|
||
| @Entity(id: 1, uid: 1) | ||
| class Note { | ||
| @Id(id: 1, uid: 1001, type: Type.Long) | ||
| @Id(id: 1, uid: 1001) // automatically always 'int' in Dart code and 'Long' in ObjectBox |
There was a problem hiding this comment.
IDs should be uint64 (in objectbox), preferrable similar in the sample code as well
There was a problem hiding this comment.
Do you mean I should change the comment to "...and 'uint64' in ObjectBox"?
There was a problem hiding this comment.
Generated code should pass flags: unsigned & ID
Originally I also meant the entity class should use unsigned integer type but I've found out there's none in dart.
There was a problem hiding this comment.
FYI: "int" in Dart is the closest type to ObjectBox IDs. The JS limitations are funny though, btw...
There was a problem hiding this comment.
Generated code should pass flags: unsigned & ID
PropertyFlags_ID (1) is enough; haven't seen PropertyFlags_UNSIGNED (8192) with IDs, better not try this?
There was a problem hiding this comment.
Right, I haven't really seen PropertyFlags_UNSIGNED before either... Can this conversation be closed then?
| """; | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
Maybe break this method up a little? I feel this getting too long.
There was a problem hiding this comment.
I would prefer to keep it like this for now; as the file is more or less linearly constructed, reading the code from top to bottom makes enough sense here. Of course this will need to change in upcoming versions of the binding, but currently, I'd say that splitting is not really necessary yet.
|
Went through all commits (skipped dropped mobile example and the C library stuff I'm not familiar with), added comments. Great! Looks like Ivan had a look as well. As I'm not around the rest of the week, I guess all follow-up questions go to him. 💃 |
We don't have libs ready for mobile platforms yet (C API, not e.g. JNI) |
Co-Authored-By: Ivan Dlugos <[email protected]>
No description provided.