azriel91 / peace

Zero Stress Automation
https://peace.mk
Apache License 2.0
110 stars 1 forks source link

`ParamsSpec`: Store Mapping Functions Separately #156

Closed azriel91 closed 8 months ago

azriel91 commented 1 year ago

Enables the developer to not have to pass in params specs for items that had mapping functions.

Currently ParamsSpec and related types -- ParamsSpecFieldless, ValueSpec, FieldWiseSpec (generated by proc macro) -- hold MappingFns within their type.

This forces users to respecify the params specs with mapping functions in subsequent CmdCtx::builder_* instantiation.

This change is to:

  1. Get developers to instantiate a Map<MappingFnK, Box<dyn MappingFn>>, and pass that to CmdCtx::builder_*.
  2. ParamsSpec and related types will hold the MappingFnK.
  3. Developers should define the MappingFnK, an all-unit-variant enum with sensible names to identify the mapping function.
azriel91 commented 8 months ago

Considering the value of this.

Summary

With it Without it
Runtime error on mismatched types Compilation error on mismatched types
Need to define extra enum Don't need to define extra enum
Don't need to specify params spec for "used what was set last time" for mapping functions -- always set in cmd ctx May forget specifying "used what was set last time" for mapping functions, and hit runtime error

With It

Developers

  1. Define an enum to name the function keys:

    enum MappingFunctions {
        BucketNameFromBucketState,
        IamPolicyArnFromIamPolicyState,
    }
  2. Define the mappings:

    let mapping_functions = {
        let mut mapping_functions = MappingFunctions::new();
        mapping_functions.insert(BucketNameFromBucketState, S3BucketState::bucket_name);
        // ..
    
        mapping_functions
    };

    Note:

    If we want to have a compilation error here, the MappingFunctions::insert function needs to have a type parameter that tells it the Item > Params > Field that the mapping function is for.

    However, developers use #[derive(Params)] to derive the <ItemParams>FieldWiseSpec, and requiring them to specify something like the following is arduous:

    MappingFunction::insert<FromType, ToType>(BucketNameFromBucketState, S3BucketState::bucket_name)
  3. Pass MappingFunctions to CmdCtxBuilder, for each code instantiation (may be just one):

    cmd_ctx_builder.with_mapping_functions(mapping_functions);
  4. Not have to call .with_item_params::<TheItem>(..) in subsequent calls.

Users

  1. Get runtime error if the mapping function type doesn't match, but it should be caught by tests.

Framework Maintainers

  1. MappingFunctions map will have magic logic to store the function argument types and return type.
  2. Error reporting when types are mismatched.

Without It

Developers

  1. Define the item params spec:

    // First execution
    let s3_object_params_spec = S3ObjectParams::<WebApp>::field_wise_spec()
        .with_file_path(web_app_path_local)
        .with_object_key(object_key)
        .with_bucket_name_from_map(S3BucketState::bucket_name)
        .build();
    
    // Subsequent executions
    let s3_object_params_spec = S3ObjectParams::<WebApp>::field_wise_spec()
        .with_bucket_name_from_map(S3BucketState::bucket_name)
        .build();
  2. Pass the item params spec to CmdCtxBuilder, for every separate code instantiation:

    cmd_ctx_builder
        .with_item_params::<S3ObjectItem<WebApp>>(
            item_id!("s3_object"),
            s3_object_params_spec,
        )

    This is somewhat of an inconvenience, because if this isn't done, the user / developer will have a runtime error, which looks like this:

    peace_rt_model::params_specs_mismatch
    
      × Item params specs do not match with the items in the flow.
      help: The following items either have not had a params spec provided previously,
            or had contained a mapping function, which cannot be loaded from disk.
    
            So the params spec needs to be provided to the command context for:
    
            * s3_object

When the closure passed to with_*_from_map doesn't have the argument type specified, or mismatches, the compilation error is still unclear. https://github.com/rust-lang/rust/pull/119888 will allow us to return a useful compilation error.

Users

No runtime error, because it will be caught at compile time.

Framework Maintainers

  1. Error messages / diagnostics showing which CmdCtx is missing which item spec for which field, should be made clear.