realm-dart icon indicating copy to clipboard operation
realm-dart copied to clipboard

Generated RealmObject constructor should use named instead of positional argument

Open calvintam236 opened this issue 2 years ago • 18 comments

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,
)

calvintam236 avatar Mar 11 '22 07:03 calvintam236

@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.

blagoev avatar Mar 11 '22 07:03 blagoev

@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.

calvintam236 avatar Mar 11 '22 08:03 calvintam236

@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

blagoev avatar Mar 11 '22 09:03 blagoev

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...

calvintam236 avatar Mar 11 '22 09:03 calvintam236

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.

lukaspili avatar Jun 16 '22 21:06 lukaspili

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.

desistefanova avatar Jun 23 '22 13:06 desistefanova

Why not use named required params?

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

lukaspili avatar Jun 23 '22 15:06 lukaspili

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!

dotjon0 avatar Jun 23 '22 15:06 dotjon0

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!

desistefanova avatar Jun 23 '22 15:06 desistefanova

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

dotjon0 avatar Jun 23 '22 17:06 dotjon0

removed as incorrect issue

dotjon0 avatar Jun 30 '22 22:06 dotjon0

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.

ebelevics avatar Nov 27 '22 21:11 ebelevics

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 ;-)

dotjon0 avatar Nov 27 '22 23:11 dotjon0

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

armandsLa avatar Sep 15 '23 08:09 armandsLa

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.

jrlcastel avatar Feb 27 '24 15:02 jrlcastel

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.

nirinchev avatar Feb 27 '24 15:02 nirinchev

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.

nielsenko avatar Feb 27 '24 15:02 nielsenko

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,
        );
}

Satishpokala124 avatar Feb 29 '24 09:02 Satishpokala124

Is there any ETA? This is painful.

dfdgsdfg avatar Apr 15 '24 06:04 dfdgsdfg

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)

dotjon0 avatar May 01 '24 19:05 dotjon0

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

dfdgsdfg avatar May 02 '24 06:05 dfdgsdfg

@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.

nielsenko avatar May 02 '24 06:05 nielsenko

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

dotjon0 avatar May 02 '24 15:05 dotjon0

@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?

nielsenko avatar May 03 '24 07:05 nielsenko

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.

dotjon0 avatar May 03 '24 16:05 dotjon0

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.

nirinchev avatar May 03 '24 18:05 nirinchev

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.

dotjon0 avatar May 05 '24 15:05 dotjon0

@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.

nielsenko avatar May 05 '24 15:05 nielsenko

@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.'

dotjon0 avatar May 05 '24 15:05 dotjon0

@dotjon0 realmModel is const?

nielsenko avatar May 05 '24 15:05 nielsenko