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

Generated RealmObject constructor should use named instead of positional argument #292

Closed
calvintam236 opened this issue Mar 11, 2022 · 37 comments · Fixed by #1641
Closed

Comments

@calvintam236
Copy link

For example, you have a datamodel class like this:

@RealmModel()
class _Hello {
  @PrimaryKey()
  final String key;

  final String sth1;

  final String sth2;

  final String sth3;

  final List<String> listA;

  final List<String> listB;
}

and this will generate a RealmObject class with this constructor:

class Hello extends _Hello with RealmObject {
  Hello(
    String key,
    String sth1,
    String sth2,
    String sth3, {
    Iterable<String> listA = const [],
    Iterable<String> listB = const [],
  }) {
    RealmObject.set(this, 'key', key);
    RealmObject.set(this, 'sth1', sth1);
    RealmObject.set(this, 'sth2', sth2);
    RealmObject.set(this, 'sth3', sth3);
    RealmObject.set<List<String>>(this, 'listA', listA.toList());
    RealmObject.set<List<String>>(this, 'listB', listB.toList());
  }
  ...
}

Now, when you create a new Hello object, you need to know exactly the order of the arguments of the constructor:

Hello(key, sth1, sth2, sth3, listA, listB);

THIS IS NOT FRIENDLY.

It should simply use named arguments in the constructor so order of the arguments does not matter & provide some flexibility (like skipping listA for default value and passing listB at the same time):

Hello(
  key: key,
  ...
  sth3: sth3,
  listB: listB,
)
@blagoev
Copy link
Contributor

blagoev commented Mar 11, 2022

@calvintam236 Thanks for posting.
This is true only for required properties. If you have a nullable (optional in Realm) property in your model than it is generated as named. The order should be the same as defined in your data model.
Also is this really an issue if you have intellisense in the editor.

EDIT: for non nullable properties with default values we do generate named arguments as well.

@calvintam236
Copy link
Author

@calvintam236 Thanks for posting. This is true only for required properties. If you have a nullable (optional in Realm) property in your model than it is generated as named. The order should be the same as defined in your data model. Also is this really an issue if you have intellisense in the editor.

EDIT: for non nullable properties with default values we do generate named arguments as well.

Thanks for getting back.

  1. Even they are in the same order as that data model class, you still need to open that file to confirm all arguments are in correct order.
  2. If I have 10 positional arguments in one constructor, does that make sense to take them positional?
  3. Please also keep in mind that NOT all developers use IDE to code. (I'm using code editor and run analyze tool on terminal. A lot of IDEs out there are performance heavy.)

See this quote from Effective Dart: In Dart, optional parameters can be either positional or named, but not both.

In fact, you can just make the constructor with required named arguments:

  Hello({
    required String key,
    required String sth1,
    required String sth2,
    required String sth3,
    Iterable<String> listA = const [],
    Iterable<String> listB = const [],
  })

This would just better for everyone.. Even pre-alpha way Hello()..key = key..sth1 = sth1 is way better than using positional arguments.

@blagoev
Copy link
Contributor

blagoev commented Mar 11, 2022

@calvintam236 Certainly there are choices to be made and we try to optimize for the majority of the use cases.

We have indeed consider your suggestion when we implemented the code generator and decided not to implement it this way, since this will require even more typing that it needs to be. Most importantly it hurts the experience in the majority of the cases where there limited number required arguments. for example:

class _Person {
  @PrimaryKey()
  final String name;
}
//this
var person = Person(name: "Myname"); 
//instead of this
var person = Person("MyName")

We believe we struck a good middle ground with the generated code and majority of the developers use an IDE these days so it helps in the rare case of 10 or more required fields.

cheers

@calvintam236
Copy link
Author

Instead of changing Person(), how about adding an alternative constructor Person.named()?

If you are using IDE, it will autocomplete as Person(name: xxx). See this.

The code should always be CLEAR to read and understand. That's the priority. Inconvenience is second.

class _Person {
  @PrimaryKey()
  final String firstname;
  final String lastname;
}
Person("last name", "first name"); // difficult to identify
Person(firstname: "last", lastname: "first"); // easy to identify this is WRONG

Confusing fields like this example will often happen. This is avoidable yet decided not to...

@lukaspili
Copy link

lukaspili commented Jun 16, 2022

There is a long discussion on dart repo for a similar topic, about having required named by default instead of positional params. There are no clear consensus, but lots of people seem to prefer using named parameters.

IMO it would be best to at least allow an option on the generator to choose between positional or named parameters.

@desistefanova
Copy link
Contributor

Looking at this example we can see that having named arguments for required fields insists to have default values for these fields, since they can not be null.

class _PersonRequiredFields {
  final String firstname;
  final String lastname;
}
class PersonRequiredFields {
  //generated realm object class constructors
  PersonRequiredFields(String this.firstname, String this.lastname);
  PersonRequiredFields.named({String this.firstname="", String this.lastname=""});// ->in this case 
  // the generator has to set the default values. Otherwise, the compiler will fail.
}

void main() { 
  PersonRequiredFields("Mark", "Parker");
  PersonRequiredFields.named(firstname: "Mark"); // lastname becomes empty even though it is required
}

If we change the realm model generator to generate named constructor for required fields, this means that the generator should provide itself these default values. It is not correct the generator to decide instead of you what the default values to be.

For this reason if you prefer having named arguments we will suggest you to define your realm model as follow:

class _PersonOptionalFields {
  // you are able to set explicitly the desired default values
  final String? firstname = "default name"; 
  final String? lastname = "default name";
}
class PersonOptionalFields {
  //generated realm object class constructor  
  PersonOptionalFields({String? this.firstname, String? this.lastname});
}

void main() { 
  PersonOptionalFields(firstname: "Mark", lastname: "Parker");
  PersonOptionalFields(firstname: "Mark");
}

In the sample above the generator will generate the exact constructor with named arguments that you prefer. But also allows you to set a specific default values.

In case you need to have a setter generated for lastname and firstname you can use late keyword as follow.

class _PersonOptionalFields {
  late String? firstname = "default name";
  late String? lastname = "default name";
}
void main() { 
 var person = PersonOptionalFields(firstname: "Mark", lastname: "Parker");
 person.lastname = "something new";
}

I hope this will answer your request for having named arguments constructor.

@lukaspili
Copy link

Why not use named required params?

class _PersonRequiredFields {
  final String firstname;
  final String lastname;
}
class PersonRequiredFields {
  PersonRequiredFields.named({required this.firstname, required this.lastname});
}

@dotjon0
Copy link

dotjon0 commented Jun 23, 2022

Before we add our feedback on this, can we just say we are incredibly grateful to the Realm team for launching the Flutter Realm package and we are so looking forward to this moving into GA. MongoDB Atlas is amazing and so is Flutter, so making them work seamless via Realm Sync is perfect. Thank you so much.

Now for our feedback:

Named or Positional Properties, or a combination of both
There is a very similar package (in terms of JSON to/from generation) json_serializable (https://pub.dev/packages/json_serializable) which is authored by Google. In this package Google gives the 'choice to the developer' to decide whether to:

  1. make all class properties 'named' OR 'positioned' (regardless of whether they are optional properties or not)
  2. set class properties 'with' OR 'without' default values (in the case for 'required' named class properties)

We request that the Realm team have the same approach as Google, and let the developer choose the approach they would like to make, as after all, this is all very subjective to the developer's approach when building applications. In our firm, we believe:

=> named properties:

  1. increase readability for the developer & code PR review speed
  2. increase code base quality and helps prevent bugs being introduced ongoing - the property/data mapping is super clear and there is no danger of positional properties being handed in the wrong order.
  3. it is very common for DB models to have 7+ properties - for starters most will need an _id and updatedAt...
  4. etc

=> required properties

  1. omit boilerplate conditional checks to ensure certain property exists which are 'essential' for the application to run - thus automatically ensuring the class instance has all of the required fields. If you are making high quality code, the fields that should exist have to be validate & caught somewhere - ideally automatically via Flutter Realm required class properties instead of developers having to introduce large amounts of boilerplate code and associated tests...
  2. makes it simple for the developer to know which properties have to be passed without reading documentation.
  3. increases maintainability as if new or existing class properties are made 'required' later on, Flutter/Dart linters flag up all the areas you need to update in your application.
  4. etc

Required Fields with 'default values'
There are notes above suggesting that 'required' properties of a class must have a 'default' value set . We believe it should be up to the developer. The reasons for this are:

  1. MongoDB Atlas schema validation supports 'required' fields with 'no default value', so Flutter Realm should follow the same for consistency.
  2. If a class/model field is not made 'required' in both (a) MongoDB Atlas, and (b) Realm local DB, in the case when Realm Sync is in use, then this is an issue that needs resolving by the developer (i.e. make MongoDB Atlas model field required) and should be caught by an exception. If the developer is not using Realm Sync and the class/model property is 'required' this should also be caught in an exception. If the developer does not want this behaviour, then the developer can simply choose not to include required class properties...

Final thoughts
It is very clear all of the above is very subjective and specific to how each individual developer wants to work and build applications and thus the only true way is to give developers the choice to go down the routes they require. Ultimately not giving the developer the choice could prevent developers using Realm local DB in Flutter and potentially the associated MongoDB Atlas and Realm Sync... FYI our firm is on an enterprise contract with MongoDB for MongoDB Atlas and Realm Sync . These limitations (as we see it) have certainly made us think twice about using Flutter Realm and we are now 'working around' these areas and building out lots of boiler plate code/tests which goes right against our grain as we believe this to be unnecessary - its slowing our development down and bloating our codebase.

Please let us really reiterate how grateful we are for the Realm Team's efforts - please keep them coming! - and we hope you will kindly consider our thoughts above. Thanks again!

@desistefanova
Copy link
Contributor

Thank you so much for giving us all these nice feedbacks. It seems to be important and more convenient for the developers to use named arguments for the required fields also. After having these reasoned feedbacks we can certainly consider providing named arguments for required fields in the constructors of generated models. We will discuss this option in the team. Thanks!

@sync-by-unito sync-by-unito bot added the Needs-Attention Reporter has responded. Review comment. label Jun 23, 2022
@dotjon0
Copy link

dotjon0 commented Jun 23, 2022

Pleasure @desistefanova and we are really looking forward to your teams feedback. If you need any further info or thoughts please do reach out ;-)

@sync-by-unito sync-by-unito bot removed the Needs-Attention Reporter has responded. Review comment. label Jun 28, 2022
@dotjon0
Copy link

dotjon0 commented Jun 30, 2022

removed as incorrect issue

@ebelevics
Copy link

ebelevics commented Nov 27, 2022

I just wanted to create same issue about this topic but I found fortunately this. For me it's bezare that every time I have to reorder parameters in RealmObject creation, when I add or change fields. If all were named parameters there would not be such problems.
And most generated db (drift atm comes in my mind) solutions I worked before, had all named parameters, not optional named, required positioned.

@dotjon0
Copy link

dotjon0 commented Nov 27, 2022

I just wanted to create same issue about this topic but I found fortunately this. For me it's bezare that every time I have to reorder parameters in RealmObject creation, when I add or change fields. If all were named parameters there would not be such problems. And most generated db (drift atm comes in my mind) solutions I worked before, had all named parameters, not optional named, required positioned.

a temp workaround we have used (which is not idea as its extra boilerplate that should not be needed) is creating a function with named properties which then returns an instance of the relevant RealmObject to then write to the DB... We are hoping Flutter Realm will support named properties for both required and non-required properties as in our feedback above soon. Hope this helps ;-)

@armandsLa
Copy link

This is a must! Just moved from sqflite to Realm and object construction is a huge drawback.

@jrlcastel
Copy link

Migrated from Hive to Realm. With the case of not being able to use inheritance (extends keyword), I moved a lot of properties into a single class & added new logic to achieve the same functionalities as a workaround. Now I am facing this issue.

Having to use a workaround to make a workaround work isnt really friendly. Being able to optionally use named or unnamed parameters via annotations could be a great feature. We could enjoy the best of both worlds.

@nirinchev
Copy link
Member

Agree. We've received a fair amount of feedback that this would be a valuable customization point, so it is high on our backlog of tasks. While we can't commit to a timeframe for when this will ship, I do expect the team to look into it soon.

@nielsenko
Copy link
Contributor

This is the second most upvoted feature request by now, and much easier to achieve than #26, so we will make it opt-in that all arguments must be named.

@Satishpokala124
Copy link

Satishpokala124 commented Feb 29, 2024

I'm also a fan of named arguments and wasn't happy with the positional arguments that Realm generates. I am new to Dart/Flutter and have tried something like this as a workaround until named arguments are supported officially.

I appreciate it if someone could provide a suggestion or opinion on whether this is ok.

@RealmModel()
class _RealmPerson {
  late String firstName;
  late String lastName;
}

class Person extends RealmPerson {
  Person({
    required String firstName,
    required String lastName,
  }) : super(
          firstName,
          lastName,
        );
}

@dfdgsdfg
Copy link

Is there any ETA? This is painful.

@dotjon0
Copy link

dotjon0 commented May 1, 2024

Amazing this is released, thank you so much! Is there a way to set the below 'globally' once rather than in 'each Realm model file'?

const config = GeneratorConfig(ctorStyle: CtorStyle.allNamed);
const realmModel = RealmModel.using(baseType: ObjectType.realmObject, generatorConfig: config);

(from https://github.com/realm/realm-dart/releases/tag/2.2.0)

@dfdgsdfg
Copy link

dfdgsdfg commented May 2, 2024

Great job! By the way, need global config for all models.

@nielsenko
Copy link
Contributor

@dotjon0 I would just define:

const config = GeneratorConfig(ctorStyle: CtorStyle.allNamed);
const realmModel = RealmModel.using(baseType: ObjectType.realmObject, generatorConfig: config);

in a separate file, and import that where you define your model.

@dotjon0
Copy link

dotjon0 commented May 2, 2024

Thanks @nielsenko, that helps. Would be best to set this globally if possible rather than having to import in each model.

@nielsenko
Copy link
Contributor

@dotjon0, @dfdgsdfg, and @jimisola.

You need to import at least package:realm/realm.dart anyway, so if you are opposed to having two imports you can create a myrealm.dart file like this:

export 'package:realm/realm.dart';
import 'package:realm_common/realm_common.dart';

const _config = GeneratorConfig(ctorStyle: CtorStyle.allNamed);
const realmModel =
    RealmModel.using(baseType: ObjectType.realmObject, generatorConfig: _config);

and then import that instead in your models. Example x.dart:

import 'myrealm.dart';

part 'x.realm.dart';

@realmModel
class _Stuff {
  late ObjectId _id;
}

I hope this is convenient enough?

@dotjon0
Copy link

dotjon0 commented May 3, 2024

Thanks @nielsenko appreciate it. Would be better in a global configuration file rather than having to remember to put this in - but then again, we are very grateful to have this functionality in any form.

@nirinchev
Copy link
Member

If you're worried about someone fat-fingering lowercase vs uppercase r, you could also give it a more distinctive name like NamedRealmModel or DotOnModel to make it less likely to confuse with the built-in RealmModel.

@dotjon0
Copy link

dotjon0 commented May 5, 2024

Hi @nirinchev would imagine a repo will either use the 'named approach' or 'positional approach' for Realm, not both. So setting it globally probably makes a ton of sense rather than setting it individually for each model. The additional import works, but again, its perhaps a workaround. Despite this, we are grateful for the support, so thank you.

@nielsenko
Copy link
Contributor

@dotjon0 In hindsight we should probably have had named arguments be the default. But history is history, and we are not going to break the existing behaviour for everyone now - I'm sorry.

For those like you who wants named arguments all over it should be a fairly simple search-replace operation to switch the actual annotation, and add an extra import (or replace as suggested above).

We can look into supporting some out-of-code json based global configuration later.

@dotjon0
Copy link

dotjon0 commented May 5, 2024

@dotjon0, @dfdgsdfg, and @jimisola.

You need to import at least package:realm/realm.dart anyway, so if you are opposed to having two imports you can create a myrealm.dart file like this:

export 'package:realm/realm.dart';
import 'package:realm_common/realm_common.dart';

const _config = GeneratorConfig(ctorStyle: CtorStyle.allNamed);
const realmModel =
    RealmModel.using(baseType: ObjectType.realmObject, generatorConfig: _config);

and then import that instead in your models. Example x.dart:

import 'myrealm.dart';

part 'x.realm.dart';

@realmModel
class _Stuff {
  late ObjectId _id;
}

I hope this is convenient enough?

Tried this but get lint error 'Annotation must be either a const variable reference or const constructor invocation.'

@nielsenko
Copy link
Contributor

@dotjon0 realmModel is const?

@dotjon0
Copy link

dotjon0 commented May 5, 2024

@dotjon0 realmModel is const?

yep it is, odd the linter is throwing this up.

@nielsenko
Copy link
Contributor

@dotjon0 Not sure what is happening on your end. Here is a small sample project showing it works.
named_args.zip

@dotjon0
Copy link

dotjon0 commented May 5, 2024

Thanks for the example project, helped us find the scenario where this is happening. So if you update the main.dart file in your example to the below the issue should be reproduced:

import 'package:flutter/material.dart';
import 'package:named_args/myrealm.dart';

part 'main.realm.dart';

@realmModel
class _Person {
  late String name;
  late int age;
  late List<_Cat> cats;
}

@realmModel(ObjectType.embeddedObject)
class _Cat {
  late String name;
}

final realm = Realm(Configuration.local([]));

void main() {
  runApp(const Placeholder());
}

Screenshot 2024-05-05 at 20 32 51

@nielsenko
Copy link
Contributor

@realmModel(ObjectType.embeddedObject) // <-- this is not valid dart

You should define a separate const var, like:

export 'package:realm/realm.dart';
import 'package:realm_common/realm_common.dart';

const _config = GeneratorConfig(ctorStyle: CtorStyle.allNamed);
const realmModel =
    RealmModel.using(baseType: ObjectType.realmObject, generatorConfig: _config);
const embeddedModel = 
    RealmModel.using(baseType: ObjectType.embeddedObject, generatorConfig: _config);

and use that like:

@embeddedModel
class _Cat {
  late String name;
}

@dotjon0
Copy link

dotjon0 commented May 5, 2024

Thanks @nielsenko, thanks works, appreciated.

FYI we took this from Realm Flutter docs at https://www.mongodb.com/docs/atlas/device-sdks/sdk/flutter/realm-database/model-data/data-types/#std-label-flutter-embedded-objects - see, where there is @RealmModel(ObjectType.embeddedObject) below.

// The generated `Address` class is an embedded object.
@RealmModel(ObjectType.embeddedObject)
class _Address {
  late String street;
  late String city;
  late String state;
  late String country;
}

@RealmModel()
class _Person {
  @PrimaryKey()
  late ObjectId id;

  late String name;

  // Embedded object in parent object schema
  late _Address? address; // Must be nullable
}

@nielsenko
Copy link
Contributor

@dotjon0 The docs are correct. The unary @ annotation operator takes a const constructed instance (ie. an instance constructed at compile time).

RealmModel is a class with a const constructor, hence it can be used to create such an instance, as happens in:

// The generated `Address` class is an embedded object.
@RealmModel(ObjectType.embeddedObject)
class _Address {
  // ..
}

and

@RealmModel()
class _Person {
  // ..
}

But you can also name those instance by storing them in const variables. That is what is happening with realmModel and embeddedModel. They just exist so that you can type less. That is not really related to realm, but just how Dart annotations and const variables "works".

@dotjon0
Copy link

dotjon0 commented May 6, 2024

Thanks @nielsenko appreciated.

Would it make sense to possibly have 2 x versions of @RealmModel() in Flutter Realm SDK? i.e. we could have @RealmModel() and @RealmModelNamed() in Realm Flutter SDK? I know we could make this, but it perhaps makes sense to keep it all centralised in the Realm Flutter SDK. Realm Flutter SDK consumers could then use CI/CD to force using either @RealmModel() or @RealmModelNamed() in their repos. So this then gives us:

@RealmModel()
class _Person {
  // ..
}

@RealmModel(ObjectType.embeddedObject)
class _Address {
  // ..
}

or for the named variation:

@RealmModelNamed()
class _Person {
  // ..
}

@RealmModelNamed(ObjectType.embeddedObject)
class _Address {
  // ..
}

Or we could potentially (for the named):

@RealmModel(CtorStyle.allNamed)
class _Person {
  // ..
}

@RealmModel(CtorStyle.allNamed, ObjectType.embeddedObject)
class _Address {
  // ..
}

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.