Skip to content
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

ParamsSpec: Store Mapping Functions Separately #156

Closed
azriel91 opened this issue Jun 28, 2023 · 1 comment · Fixed by #183
Closed

ParamsSpec: Store Mapping Functions Separately #156

azriel91 opened this issue Jun 28, 2023 · 1 comment · Fixed by #183
Assignees

Comments

@azriel91
Copy link
Owner

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 azriel91 changed the title ParamsSpecs: Store Mapping Functions Separately ParamsSpec: Store Mapping Functions Separately Jun 28, 2023
@azriel91 azriel91 self-assigned this Feb 2, 2024
@azriel91
Copy link
Owner Author

azriel91 commented Feb 2, 2024

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. rust-lang/rust#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant