Skip to content

Generate drift tables #127

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

Closed
wants to merge 31 commits into from

Conversation

ikbendewilliam
Copy link
Contributor

No description provided.

Copy link
Contributor

@NicolaVerbeeck NicolaVerbeeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be specified that custom converters are not supported for drift (unless we add support for them)

Copy link
Contributor

@vanlooverenkoen vanlooverenkoen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if all the test cover all cases. We should maybe create a list of edgecases and check if the tests are covering those edge cases as well

@@ -0,0 +1 @@
// Should throw an error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also indicate why it should throw here?

@vanlooverenkoen
Copy link
Contributor

generate_drift_table ignore_for_table I would use ignore_for_drift_table here as well.

@vanlooverenkoen
Copy link
Contributor

 authors is an array or map. Ignore this field by adding ignore_for_table: true

improve error handling.

`authors` is an "array or maps", this is not support to save in a table. You can ignore this field by adding

   ignore_for_table: true

Also, specify if it is a map or array. We already know this.

@vanlooverenkoen
Copy link
Contributor

There is no error thrown when I create a table. without a primary key.
Maybe we don't need an error, but we should probably log this.

@vanlooverenkoen
Copy link
Contributor

Question: what happens if we want to add an extra field that is not returned from the API? And we don't want to be creating an extra table, and add complexity of doing a lookup in that extra table?

@vanlooverenkoen
Copy link
Contributor

When using a type: custom we should allow to specify a TypeConverter. Right now it just said, No drift column or type for Duration but duration could be mapped to the amount of seconds or miliseconds.

@vanlooverenkoen
Copy link
Contributor

Tables generate 1 enter to much. after all columns

@vanlooverenkoen
Copy link
Contributor

A generated table imports

import 'package:flutter/foundation.dart';

but it is never needed.

config

Book2:
  path: book/
  generate_drift_table: true
  properties:
    id:
      type: int
      required: true
      is_table_primary_key: true
    name: string

@vanlooverenkoen
Copy link
Contributor

There is 1 big bug I thinks with enums.

This is the config

    secondCategory: BookCategory?

This is generated.

class BookTableBookCategoryConverter extends TypeConverter<BookCategory, String> {
  const BookTableBookCategoryConverter();

  @override
  BookCategory fromSql(String fromDb) {
    for (final value in BookCategory.values) {
      if (value.jsonValue == fromDb) return value;
    }
    return BookCategory.values.first;
  }

  @override
  String toSql(BookCategory value) {
    return value.jsonValue;
  }
}

I would expect the BookCategory in the BookTableBookCategoryConverter to be nullable. Becuase right now you just return the first item. Which will result in incorrectly saved values.

@ikbendewilliam
Copy link
Contributor Author

A generated table imports

import 'package:flutter/foundation.dart';

but it is never needed.

config

Book2:
  path: book/
  generate_drift_table: true
  properties:
    id:
      type: int
      required: true
      is_table_primary_key: true
    name: string

Note that this is added in the config in pubspec.yaml

  extra_imports:
    - 'package:flutter/foundation.dart'

@ikbendewilliam
Copy link
Contributor Author

ikbendewilliam commented Jan 11, 2023

Question: what happens if we want to add an extra field that is not returned from the API? And we don't want to be creating an extra table, and add complexity of doing a lookup in that extra table?

I'll add only_for_table for this

@ikbendewilliam
Copy link
Contributor Author

When using a type: custom we should allow to specify a TypeConverter. Right now it just said, No drift column or type for Duration but duration could be mapped to the amount of seconds or miliseconds.

I've added extra_imports_for_table and type_converter_for_table for this

@ikbendewilliam
Copy link
Contributor Author

Added a nullable enum converter for non-required fields

@vanlooverenkoen
Copy link
Contributor

Decline for now. Can be revisited in the future

@vanlooverenkoen vanlooverenkoen deleted the experimental/generate-drift-tables branch August 30, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants