fix: allow optional `GenerationConfig` fields

[jjangga0214]

The README guides creating custom bin like this.

use std::{collections::HashMap, path::PathBuf};
use dsync::{GenerationConfig, TableOptions};

pub fn main() {
    let dir = env!("CARGO_MANIFEST_DIR");

    dsync::generate_files(
        PathBuf::from_iter([dir, "src/schema.rs"]), 
        PathBuf::from_iter([dir, "src/models"]), 
        GenerationConfig { /* ... your generation options ... */ }
    );
}

https://github.com/Wulf/dsync/blob/007ace83c139f67e1d85ed4829cf0b6953d121df/src/lib.rs#L216-L237

I wanted to only specify GenerationConfig.connection_type, but I am forced to input every options. I think they should be able to be omitted except connection_type.

Thank you very much for this precious project.

[hasezoey]

I think they should be able to be omitted except connection_type.

the problem with Default is that you can either:

• not implement it and must provide every option
• implement it and provide a default for every option

to my knowledge there is no in-between, but i guess we could add a builder method

[Wulf]

You can do this :)

GenerationConfig { override: true, ...Default::default() }

Maybe we can update the example in the README?

edit: ah wait, I forgot to push this, I'll push this now!

[hasezoey]

i have thought about this for a bit. this could be achieved in multiple ways, but i am unsure which to take:

1. the simplest option of all fields having a default (including connection_type)

   GenerationConfig { ..Default::default() }
   GenerationConfig {
   connection_type: "something else"
   ..Default::default() 
   }

2. the second simplest option of all fields having a default, but connection_type is empty and errors at runtime

   
   GenerationConfig { ..Default::default() } // Error at runtime

GenerationConfig { connection_type: "something else" ..Default::default() }

3. move all non-required (defaultable) fields into their own struct as a subfield
  ```rust
GenerationConfig {
    connection_type: "something else"
    subfields: SomeName { // or directly Default::default()
        ..Default::default()
    }
}

i am personally against erroring at runtime and somewhat against connection_type having a default.

(using the builder pattern would also be a option, but i didnt list it because it is quite verbose)

any other suggestions?

[Wulf]

I agree, we should be treating required and optional fields differently. Which part of the builder pattern do you feel is verbose?

For example, I don't think this is so bad:

GenerationConfig::new(required1, required2, ...)
  .optional1(..)
  .optional2(..)

I like this more than writing ..Default::default()

[hasezoey]

Which part of the builder pattern do you feel is verbose?

if we should ever do more required fields, then it would get more and more arguments to the "new" function, and the parameters (without inlay code hints) would not be quite obvious what each parameter is compared to fieldA: variableA. another option would be to use a hybrid approach (BuilderConfig { requiredFields }.builderA()), but at that point why not just use point 3 of the comment.

in contrast to my previous point, currently there is only 1 required option (to my knowledge), that being connection_type, everything else has defaults.

also just writing the builder pattern is quite verbose of repeated pub fn something(self, arg) { self.arg = arg }.

For example, I don't think this is so bad:

it is not bad if there are not many options, but consider the current main.rs, it would look something like:

GenerationConfig::new(connection_type)
    .default_table_options(default_table_options)
    .table_options(HashMap::from([]))
    .connection_type(args.connection_type)
    .schema_path(args.schema_path)
    .model_path(args.model_path)
    .once_common_structs(args.once_common_structs)
    .once_connection_type(args.once_connection_type)
    .readonly_prefixes(args.readonly_prefixes)
    .readonly_suffixes(args.readonly_suffixes);

instead of just

GenerationConfig {
    default_table_options,
    table_options: HashMap::from([]),
    connection_type: args.connection_type,
    schema_path: args.schema_path,
    model_path: args.model_path,
    once_common_structs: args.once_common_structs,
    once_connection_type: args.once_connection_type,
    readonly_prefixes: args.readonly_prefixes,
    readonly_suffixes: args.readonly_suffixes,
},

also tooling would allow the current way to be formatted like

GenerationConfig {
    default_table_options,
    table_options:          HashMap::from([]),
    connection_type:        args.connection_type,
    schema_path:            args.schema_path,
    model_path:             args.model_path,
    once_common_structs:    args.once_common_structs,
    once_connection_type:   args.once_connection_type,
    readonly_prefixes:      args.readonly_prefixes,
    readonly_suffixes:      args.readonly_suffixes,
},

though i dont know if this is a valid point

---

expanding more on the point 3, it would look currently something like

GenerationConfig {
    connection_type: args.connection_type,
    optional: GenerationConfigOpt {
        default_table_options,
        table_options: HashMap::from([]),
        schema_path: args.schema_path,
        model_path: args.model_path,
        once_common_structs: args.once_common_structs,
        once_connection_type: args.once_connection_type,
        readonly_prefixes: args.readonly_prefixes,
        readonly_suffixes: args.readonly_suffixes,
    }
};

GenerationConfig {
    connection_type: args.connection_type,
    optional: GenerationConfigOpt::default()
};

---

a hybrid approach could look something like:

GenerationConfig::new(connection_type)
    .with_struct(GenerationConfigOpt {
        default_table_options,
        table_options: HashMap::from([]),
        schema_path: args.schema_path,
        model_path: args.model_path,
        once_common_structs: args.once_common_structs,
        once_connection_type: args.once_connection_type,
        readonly_prefixes: args.readonly_prefixes,
        readonly_suffixes: args.readonly_suffixes,
    });

while still allowing calling individual function to only overwrite specific options

---

also if we should go with a builder pattern, should we make it a simple version where each GenerationConfig is valid, or should we make a specific builder and a .build function? for the lack of better expressing it, examples:

// "simple"
let config = GenerationConfig::new(connection_type);

useage_fn(config);

// with ".build" function
let config = GenerationConfigBuilder::new(connection_type).build();

useage_fn(config);

[Wulf]

ah okay, understood. I can see why you like the approach in point 3 :)

What if we split the required and optional structs entirely?

GenerationConfig {
  // required fields
}

GenerationOptions {
  // optional fields, all of these have sane defaults
}

Then we can pass this in directly into the library's main fn:

dsync::generate_files(
  input, 
  output, 
  // here are the required args:
  GenerationConfig {},
  // here are the optional args:
  Default::default()
)

Perhaps this way we can also move input and output into the config struct

[hasezoey]

What if we split the required and optional structs entirely? Then we can pass this in directly into the library's main fn:

this would be a approach, but this would mean to bring 2 arguments across multiple functions (down to functions in code.rs), which instead of just having 1 struct to pass-around.

i would be in favor of having this as a field rather than completely separate (and maybe the hybrid approach i wrote about earlier)

Perhaps this way we can also move input and output into the config struct

the input(input_diesel_schema_file) and output(output_models_dir) only concerns lib::generate_files because that function deals with the reading & saving of files, it is not used anywhere else; we also have other public API that uses GenerationConfig, but completely does not need those arguments, like lib::generate_code

[Wulf]

ah okay! I'm in favour of the third approach you outlined above (point 3) but ultimately, I'll leave the decision to you :)

Also, if you do want to go that route, could I suggest we name it opts or options instead of optional?

Thanks again for exploring this in a detailed manner!

[hasezoey]

Also, if you do want to go that route, could I suggest we name it opts or options instead of optional?

sure will make a test PR and then we will see if it is a good approach and what can be changed