6thsolution / dart_sealed

Dart and Flutter sealed class generator and annotations, with match methods and other utilities. There is also super_enum compatible API.
https://pub.dev/packages/sealed_annotations
MIT License
17 stars 9 forks source link

Common fields #8

Closed FatulM closed 3 years ago

FatulM commented 3 years ago

add capability to add common fields. for example:

import 'package:sealed_annotations/sealed_annotations.dart';

part 'common.sealed.dart';

@Sealed()
abstract class _ApiError {
  String get message;

  String? get code;

  void internetError();

  void badRequest();
}

By now the best way to implement such a behavior is to use extension methods on a conventional seald. like this:

import 'package:sealed_annotations/sealed_annotations.dart';

part 'sobhan.sealed.dart';

@Sealed()
abstract class _ApiError {
  void internetError(String message, String? code);

  void badRequest(String message, String? code);
}

extension on ApiError {
  String get message => map(
        internetError: (e) => e.message,
        badRequest: (e) => e.message,
      );

  String? get code => map(
        internetError: (e) => e.code,
        badRequest: (e) => e.code,
      );
}

or by casting to dynamic in extension function:

extension on ApiError {
  String get message => (this as dynamic).message as String;

  String? get code => (this as dynamic).code as String?;
}

also the question about default values remains, for example consider internetError code is always null or any other value.

FatulM commented 3 years ago

@SaeedMasoumi

FatulM commented 3 years ago

@sobimor

FatulM commented 3 years ago

there is the possibility to use sub-class of a common field in sub-classes. for example

@Sealed()
abstract class _State {
  Object? get obj;

  void one(String obj);

  void two();

  void three(Object obj);

  void four(Object? obj);
}

and also two and four should have identical signature.

FatulM commented 3 years ago

examples:

@Sealed()
abstract class _ApiError {
  String get message;

  String? get code;

  void internetError();

  void badRequest();

  void internalError(Object? error);
}

@Sealed()
abstract class _Common {
  Object? get x;

  final String y = 'hello';

  void one(String x);

  void two(Object x);

  void three();

  void four(Object? x);
}

@Sealed()
abstract class _Result<D extends num> {
  Object? get value;

  void success(D value);

  void error();
}

@Sealed()
abstract class _ResultLeftRight<D extends num> {
  D get data;

  void successLeft();

  void successRight();
}
FatulM commented 3 years ago

writing phase is implemented , it remains to implement reading phase ... @SaeedMasoumi

FatulM commented 3 years ago

reading phase is implemented, it remains to add docs ...

FatulM commented 3 years ago

This is finished, checkout: https://github.com/6thsolution/dart_sealed/blob/master/sealed_example/bin/sealed/nullsafe/data/simple/common.dart https://github.com/6thsolution/dart_sealed/blob/master/sealed_example/bin/sealed/nullsafe/data/simple/common_complex.dart https://github.com/6thsolution/dart_sealed/blob/master/sealed_example/bin/sealed/nullsafe/data/generic/result_common.dart https://github.com/6thsolution/dart_sealed/blob/master/sealed_example/bin/sealed/nullsafe/data/generic/result_common_split.sealed.dart

FatulM commented 3 years ago

@SaeedMasoumi @sobimor

iamarnas commented 3 years ago

@FatulM Hi. Why not allow to user create a constructor to create common fields?

Example:

@Sealed()
// In any case, this class is not visible and is used to get user settings.
// But provide more details about the user preferences
abstract class _ApiError {
  // Here users have the possibility to provide default values and to avoid lint errors.
  _ApiError({this.message, this.code}); 

  final String? message;
  final String? code;

  void internetError();
  void badRequest();
  void internalError(Object? error);
}

And the override them over super constructor by new values.

class ApiErrorInternetError extends ApiError with EquatableMixin {
  const ApiErrorInternetError({
    String? message,
    String? code,
  }) : super(message: message, code: code);

  @override
  String toString() => 'ApiError.internetError(message: $message, code: $code)';

  @override
  List<Object?> get props => [message, code];
}

In this case, you are not dependent on abstract classes. Or it is a purpose to avoid constructor for superclass?

FatulM commented 3 years ago

1) about adding constructor to manifest class: I've checked out, You can add constructor and it will generate correctly. I will point out this in docs in future releases.

@Sealed()
abstract class _ApiError {
  final String message;

  final String? code;

  _ApiError(this.message, this.code);

  void internetError();

  void badRequest();

  void internalError(Object? error);
}

2) about adding constructor with common fields on super class: Adding common fields to super class and calling super constructor in sub classes, has two downsides:

3) about making super class not abstract: It is achivable without this, for example by adding an empty sealed method:

@Sealed()
abstract class _ApiError {
  String get message;

  String? get code;

  void general();

  void internetError();

  void badRequest();

  void internalError(Object? error);
}

here it is named for example general then you can use ApiError.general() like when ApiError is not abstract.

4) Adding constructors to manifest classes will add more complexity, the most important property of dart_sealed is simplicity, about default values it can be added using

abstract class _ApiError {
final String message = 'lollipop';
}

or

abstract class _ApiError {
@WithDefault('lollipop')
String get message;
}

Thank you for your recommendations

@iamarnas @SaeedMasoumi

iamarnas commented 3 years ago

@FatulM By generating common fields like in this example.

@Sealed()
abstract class _ApiError {
  String get message;

  String? get code;

  void internetError();

  void badRequest();

  void internalError(Object? error);
}

You don't need to add them to the methods...

R when<R extends Object?>({
    required R Function(String message, String? code) internetError, // <- common fields not need here.
    required R Function(String message, String? code) badRequest, // <- common fields not need here.
    required R Function(String message, String? code, Object? error) // <- common fields not need here.
        internalError, 
  }) {
    final apiError = this;
    if (apiError is ApiErrorInternetError) {
      return internetError(apiError.message, apiError.code);
    } else if (apiError is ApiErrorBadRequest) {
      return badRequest(apiError.message, apiError.code);
    } else if (apiError is ApiErrorInternalError) {
      return internalError(apiError.message, apiError.code, apiError.error);
    } else {
      throw AssertionError();
    }
  }

Note: Think what it will look like after a long list of common fields :)

That's why is the map method is useful in this case. The map method provides all union classes separately and you can read or override all properties from the class, here I don't need to explain... But I would recommend implementing copyWith a method to make a better experience and cleaner code with immutability.

Example how it would look:

ApiError apiError(ApiError api) {
  return api.map(
    internetError: (internetError) => internetError.copyWith(message: 'Internet error'), // <- common fields.
    badRequest: (badRequest) => badRequest.copyWith(code: '400'), // <- common fields.
    internalError: (internalError) => internalError.copyWith(error: Error()), // <- union
  );
}

/// Even with when the method you can return common field by using `orElse()`
ApiError apiError(ApiError api) {
  return api.maybeWhen(
    internetError: () {
      return const ApiErrorInternetError(message: 'no connection');
    },
    badRequest: () {
      return const ApiErrorBadRequest(message: 'bad request', code: '400');
    },
    internalError: (error) {
      return ApiErrorInternalError(message: 'Some error', error: error);
    },
    orElse: (apiError) {
      return ApiErrorBadRequest(message: apiError.message, code: apiError.code);
    },
  );
}

Your generated classes are immutable and in all case, copyWith method makes life easier with immutability.

FatulM commented 3 years ago

About removing common fields from when lambdas: 1 What about when user decides to use sub class of common field In sealed classes ?

2 Then for accessing common field he should use the variable which he had used to match against, isn't this counter intuitive?

About copy with method: I agree that we need a copy method, But implementating it when we have generics and nullable fields is very hard or impossible...

For example consider this:

class Base<T extends Object> {
void one(T x, T y);
}

Can you provide a implementation for copy with ?

iamarnas commented 3 years ago

About copy with method: I agree that we need a copy method... For example consider this:

class Base<T extends Object> {
void one(T x, T y);
}

Can you provide a implementation for copy with ?

Just add simple copyWith method to every union class. And in every class same value be overridden. Of course every union class have different parameters. In this cace in the class One would look like this.

//... 
@override
final int a; // <- common field. 
final T x;
final T y;

// `T x` and `T y` here is different values.
One copyWith({int a, T x, T y}) => One(
        a: a ?? this.a,
        x: x ?? this.x,
        y: y ?? this.y,
      ); 

You can ignore generate copyWith method only if no parameters provided. @FatulM

FatulM commented 3 years ago

You mean that we disallow changing generic type with copy with method ? It's a good idea 😃

What about nullable fields, for example:

class Base{
void one(int? x);
}
iamarnas commented 3 years ago

@FatulM Have you tried it? With null-safety should return null as default. With non null it is little boilerplate. Freezed allow null with copyWith method.

FatulM commented 3 years ago

please check this out:

import 'package:freezed_annotation/freezed_annotation.dart';

part 'main.freezed.dart';

@freezed
class Base with _$Base {
  const factory Base.one({int? x}) = _One;

  const factory Base.two() = _Two;
}

this will generate code which needs two levels of inheritance for each copy with method and is not very straight forward.

we have this:

/// @nodoc
abstract class $BaseCopyWith<$Res> {
  factory $BaseCopyWith(Base value, $Res Function(Base) then) =
      _$BaseCopyWithImpl<$Res>;
}

/// @nodoc
class _$BaseCopyWithImpl<$Res> implements $BaseCopyWith<$Res> {
  _$BaseCopyWithImpl(this._value, this._then);

  final Base _value;

  // ignore: unused_field
  final $Res Function(Base) _then;
}

then this:

/// @nodoc
abstract class _$OneCopyWith<$Res> {
  factory _$OneCopyWith(_One value, $Res Function(_One) then) =
      __$OneCopyWithImpl<$Res>;

  $Res call({int? x});
}

/// @nodoc
class __$OneCopyWithImpl<$Res> extends _$BaseCopyWithImpl<$Res>
    implements _$OneCopyWith<$Res> {
  __$OneCopyWithImpl(_One _value, $Res Function(_One) _then)
      : super(_value, (v) => _then(v as _One));

  @override
  _One get _value => super._value as _One;

  @override
  $Res call({
    Object? x = freezed,
  }) {
    return _then(_One(
      x: x == freezed
          ? _value.x
          : x // ignore: cast_nullable_to_non_nullable
              as int?,
    ));
  }
}

we have this:

@JsonKey(ignore: true)
  _$OneCopyWith<_One> get copyWith => throw _privateConstructorUsedError;

which will be overriden by:

  @JsonKey(ignore: true)
  @override
  _$OneCopyWith<_One> get copyWith =>
      __$OneCopyWithImpl<_One>(this, _$identity);

I myself had this solution in mind but is very time consuming and complex to generate.

iamarnas commented 3 years ago

Like I said it boilerplate 😁 But or it's need with Dart null-safety? Freezed was created before null safety.

FatulM commented 3 years ago

I think it is needed ...

iamarnas commented 3 years ago

I think it is needed ...

@override
final int? a;
final T x;
final T y;

One copyWith({
    int? a, /// <- Compiler tell to the user about null directly from the `copyWith` method.
    T x,
    T y, 
}) {
return One(
    a: a ?? this.a,
    x: x ?? this.x,
    y: y ?? this.y,
  ); 
} 

@FatulM

FatulM commented 3 years ago

In my example, In copy with method, If you pass null to x or don't pass any thing, They are the same and it does not change x.

Think of an example where x is 10 and you want to change it to null by using copy with method. It can't be done ...

Freezed uses this complex mechanism to solve this issue

One copyWith({
    int? x,
}) {
return One(
    x: x ?? this.x,
  );
}
iamarnas commented 3 years ago

Ok. Then without boilerplate is no way 🙂

iamarnas commented 3 years ago

What about this method? Found it on Stackoverflow and looks less boilerplate.

class Optional<T> {
  final bool isValid;
  final T? _value;

  T get value => _value as T;

  const Optional()
      : isValid = false,
        _value = null;
  const Optional.value(this._value) : isValid = true;
}

class Person {
  final String? name;

  Person(this.name);

  Person copyWith({Optional<String?> name = const Optional()}) =>
      Person(name.isValid ? name.value : this.name);
}

void main() {
  final person = Person("Gustavo");
  print(person.name);

  final person2 = person.copyWith(name: Optional.value(null));
  print(person2.name);
}

@FatulM Just one simple class which handle null.

FatulM commented 3 years ago

But when you want to set to any thing you should use optional ...

void main() {
  final person = Person("Gustavo");
  print(person.name);

  final person2 = person.copyWith(name: Optional.value("Fring"));
  print(person2.name);
}

Ok, I should think more about this ... Thank you for your recommendation

iamarnas commented 3 years ago

@FatulM It's possible to hide it inside the function.

class Person {
  final String? name;
  final int? age;
  final double? height;

  Person(this.name, this.age, this.height);

  Person copyWith({
    String? name,
    int? age,
    double? height,
  }) {
    Optional<String?> _name = Optional.value(name);
    Optional<int?> _id = Optional.value(age);
    Optional<double?> _height = Optional.value(height);

    return Person(
      _name.isValid ? _name.value : this.name,
      _id.isValid ? _id.value : this.age,
      _height.isValid ? _height.value : this.height,
    );
  }
}
FatulM commented 3 years ago

I think you made a mistake, Since the very first point of using optional was to use deafault value of Optional() for arguments, So argument types need to be Optoional<T>.

In your solution you can not leave any value unchanged .

iamarnas commented 3 years ago

I think you made a mistake, Since the very first point of using optional was to use deafault value of Optional() for arguments, So argument types need to be Optoional<T>.

In your solution you can not leave any value unchanged .

@FatulM You are right it does not work as expected.

FatulM commented 3 years ago

I made other issues which originates from this issue, Since this particular one is finished I will close the issue. @SaeedMasoumi

Thank you for your recommendations: @iamarnas @sobimor