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

feat: support conditional prompting for variables #1251

Open
1 task
alestiago opened this issue Feb 13, 2024 · 6 comments
Open
1 task

feat: support conditional prompting for variables #1251

alestiago opened this issue Feb 13, 2024 · 6 comments
Labels
customer:🦄 enhancement candidate Candidate for enhancement but additional research is needed feedback wanted Looking for feedback from the community

Comments

@alestiago
Copy link
Collaborator

alestiago commented Feb 13, 2024

Description

As a mason user, I would sometimes like to prompt for variables conditionally depending on the previous answers.

Conditional prompting can be achieved directly through the pre_gen.dart hook. However, if the prompting logic is moved to the pre generation hook the brick.yaml will no longer need to declare the variables.

Requirements

  • Support conditional prompting of variables whilst still declaring them

Additional context

  • An effort like this one is likely to require a proper proposal document.
@alestiago alestiago changed the title feat: support conditional prompting in brick.yaml feat: support conditional prompting in for variables Feb 14, 2024
@alestiago alestiago changed the title feat: support conditional prompting in for variables feat: support conditional prompting for variables Feb 14, 2024
@Farley-Chen
Copy link

I would like the new feature too.

I have try a little change of the source code to add a new option like depend-on

example like this

vars:
  injectable_enable:
    type: boolean
    default: true
    prompt: 是否声明为injectable?
  injectable:
    type: enum
    depend-on: injectable_enable
    default: "lazySingleton"
    prompt: 使用哪种类型的injectable(仅在声明为injectable时生效)?
    values:
      - "singleton"
      - "lazySingleton"

so when injectable_enable is false, the prompt of injectable will not show.

the source code I change like follow

in mason brick_yaml.dart add a new field dependOn

some change like this

/// add new code to change constructor
...
  /// An optional description of the variable.
  final String? description;

   // add new code here
  /// An optional dependency of the variable.
  final String? dependOn;

  /// An optional default value for the variable.
  @JsonKey(name: 'default')
  final Object? defaultValue;
...

in mason_cli make.dart _make()

...
      for (final entry in _brick.vars.entries) {
        final variable = entry.key;
        final properties = entry.value;
        // add new code
        if (properties.dependOn != null &&
            const [null, '', false].contains(vars[properties.dependOn])) {
          continue;
        }
        ...
     }
...

It is a easy way, but I do not know there is other design defect.

I found that yeoman support function condition, I do not know if we need it.

@felangel
Copy link
Owner

felangel commented Mar 7, 2024

Thanks for the issue! I think we need to have a more detailed proposal. The snippet from @Farley-Chen seems like a good start; however, it's unclear how this would work with non-boolean variable values (e.g. I only want to ask for variable B if the value of variable A is "foo").

There are lots of cases to consider and initially it seems to me that we'd need to support scripting within the yaml to satisfy all the requirements (similar to GitHub workflow syntax) however; I don't love having scripting directly in yaml since you lose the ability to execute, test, lint, etc.

@felangel felangel added feedback wanted Looking for feedback from the community enhancement candidate Candidate for enhancement but additional research is needed labels Mar 7, 2024
@ka-zo
Copy link

ka-zo commented Mar 13, 2024

This is a feature, I'd definitely love to see in Mason.

The proposal of @Farley-Chen is a good starting point, but has the drawback, that the depend-on parameter implicitly assumes boolean type parameter for its value (just like @felangel pointed out), in other words the proposed solution cannot handle dependency on other types of parameters. As a consequence, I'd suggest a different solution.

My Proposal

My proposal is to introduce the pre_prompt optional parameter for each variable, instead of the depend-on parameter. The value of the optional pre_prompt parameter has to be a Dart function identifier. In case the pre_prompt optional parameter exists for a variable, Mason would execute the corresponding Dart function just before the corresponding variable gets prompted to the user. The input parameter for the Dart function would always be HookContext context variable, and so the function can access the values of all input parameters in the brick.yaml file that already have a user defined value. In other words, such a function can run any kind of logic depending on the previously provided user inputs. A dart function referenced by the pre_prompt parameter can always rely on the values of the preceding variables in the brick.yaml file. Probably the return value of the function should be boolean, where true means to prompt the corresponding variable, false would mean to skip the corresponding (actual) variable and proceed to the next one (in the brick.yaml file).

This business logic allows and enables the implementation of a complete control flow of all the variables defined in the brick.yaml file. As the pre_prompt parameter is optional, the proposed solution is backwards compatible, existing brick.yaml files would still work out of the box. The Dart function identifier could be a user defined function name. Mason would look for the definition of the function in a dart file whose name is hard coded in mason, just like the name pre_gen.dart. We could call it pre_prompt.dart or prompt_flow.dart. For now, I would stick with the name pre_prompt.dart. This dart file could be located in the hooks folder of a brick.

Example

Relevant part of brick.yaml file could look like this:

vars:
  has_authentication:
    type: boolean
    description: If app should support authentication or not.
    default: true
    prompt: Do you want support for authentication?
  web_client_id:
    type: string
    description: Web client id string in order to allow authentication with Google provider.
    default: YOUR_WEBCLIENT_ID
    pre_prompt: isWebClientIdNeeded # This is the name of the Dart function that checks the value of the previous has_authentication variable. If it is false, this variable shall not be prompted for value.
    prompt: Paste here the web client id from Google Firebase Console.

The hooks/pre_prompt.dart could look like this:

import 'package:mason/mason.dart';

bool isWebClientIdNeeded(HookContext context) {
  if (context.vars['has_authentication']) {
    // prompt for web_client_id
    return true;
  }
  // skip prompting web_client_id
  return false;
}

Comparison

In the following, I compare my proposal with the existing solution, where brick developers are forced to define conditional variables in hooks, instead of the brick.yaml file.

My proposed solution:

  • Pros:
    • Brick maintenance is easier.
    • Clear separation of variable definitions and business logic for conditional variables.
    • All the variables are located in a single configuration file, the brick.yaml file.
    • All business logic of all the conditional variables (which have a pre_prompt parameter in the brick.yaml file) are collected in a single predefined Dart file, the pre_prompt.dart file, and so maintenance of the business logic is easier.
    • The proposed solution can handle any type of conditional variables (does not need to depend on boolean variables)
  • Cons:
    • Complexity is on the Mason side.
    • Further development is needed, there is a need to handle errors, like non-existing function definitions for the function identifiers provided in the pre_prompt parameters, potentially wrong return values (could be handled with signatures).

Existing solution, in which conditional variables have to be moved from the brick.yaml file to hooks, and corresponding business logic of the conditions have to be implemented also in the hooks:

  • Pros:
    • No need for further development.
  • Cons:
    • Complexity is on the Brick developer side.
    • Brick maintenance is more difficult.
    • No clear separation of variable definitions and business logic for conditional variables.
    • Developer defined variables are scattered among brick.yaml and Dart hook files, and so having an overview of existing variables is more difficult.
    • Routine tasks, like prompting needs to be handled by the brick developer for those variables, that are defined and located in the hooks and not in the brick.yaml file. Prompting should normally be done by Mason.

Potential errors of brick developers, when using my proposal

  • pre_prompt parameter has no value.
  • value of pre_prompt parameter is referencing a non-existent Dart function.
  • signature of Dart pre_prompt function is not in line with requirement (requirement: bool myPrePromptFunction(HookContext context)).
  • Brick developer does not implement business logic for both boolean return values.

@alestiago
Copy link
Collaborator Author

alestiago commented Mar 15, 2024

Hi @ka-zo ! Thanks for chiming in.

Your proposal reminds me to how Visual Studio Code extension utilise custom when clauses.

I do believe the conditional logic should be in a programming language and not a markup language such as YAML. As @felangel stated, programming languages already provide testing out of the box. Hence, I like fact that your proposal used Dart to define the conditional logic.


Decision tree

I would like to bring attention to decision trees. I think it would be very nice if, as a consumer of a brick, I could know the decision tree associated with a variable. In other words, knowing at a glance when will variable "xyz" be prompted for, or what other variables are needed when variable "abc" is provided.

If we still want to have the brick.yaml declare variables, we might be able to take advantage of indentation to convey the decision tree and scope the conditions of a variable to those ancestor variables in the hierarchy. Here is a draft example on how would the usage of indentation would define such decision tree:

vars:
  animals:
    type: enum
    values: ["none", "dogs", "cats"]
    default: "none"
    prompt: What animal do you want to adopt?
    with:
      dogs:
        type: number
        when: # How we allow specifying this condition is to be defined. (animals == dogs)
        prompt: How many dogs would you like to adopt?
        with:
          names:
            type: list
            when: # How we allow specifying this condition is to be defined. (dogs > 0)
            prompt: What should their names be?

I haven't had much time to compose an actual proposal. Just wanted to emphasize that I would value and appreciate a solution that provides something that looks like a decision tree to the user of a brick, so that they can conceptualize what variables could be prompted, and when, at a glance.


P.S. I feel that the ability of specifying custom types might be somewhat intertwined (depending on the scenario) with this conditional prompting issue.

@ka-zo
Copy link

ka-zo commented Mar 15, 2024

Hi @alestiago ,

Good point. I like your suggestion a lot, it provides an immediate overview of the dependencies for brick developers. What do you think about combining your proposal with mine: decision tree + Dart code based decisions?

Dart code based business logic for the decisions allow implementing even complex decision processes, furthermore the brick.yaml file does not get cluttered with the conditional logic. On top of that, we still have the clear separation of variables and business logic for the conditional variables. Variables are still located in the brick.yaml file, whereas business logic for the conditionals could be located in the hooks/conditions.dart file.

My example would look like this, after combining it with your proposal:

The relevant part of the brick.yaml file:

vars:
  has_authentication:
    type: boolean
    description: If app should support authentication or not.
    default: true
    prompt: Do you want support for authentication?
    with:
      web_client_id:
        type: string
        when: hasAuthentication # This is the name of the Dart function that checks the value of the previous has_authentication variable. If it is false, this variable shall not be prompted for value.
        description: Web client id string in order to allow authentication with Google provider.
        default: YOUR_WEBCLIENT_ID
        prompt: Paste here the web client id from Google Firebase Console.

The conditions could be stored in the hooks/conditions.dart file:

import 'package:mason/mason.dart';

bool hasAuthentication(HookContext context) {
  if (context.vars['has_authentication']) {
    // prompt for web_client_id
    return true;
  }
  // skip prompting web_client_id
  return false;
}

The when parameter must be a required parameter for the second level variables and above, however it must not be present in the definition of the first level variables, as first level variables are anyway the roots of the decision tree. The with parameter is optional on all levels, except, if a variable has a child variable. This makes this solution backwards compatible, and existing brick.yaml files would still work out of the box.

The signature of the dart function implementing the conditional logic can still be the same: bool myCondition(HookContext context). True return value would mean prompting the corresponding variable, false would mean not prompting.

This would allow even more complex decisions, like creating many conditional variables on the same level, each corresponding to a specific number range of the parent node of type number.

Walking through the decision tree, and prompting (or not prompting) each variable could be done in the order the variables are located in the brick.yaml file, as the brick developer probably automatically inserted the variables into the brick.yaml file in the most natural order. This way, the brick developer also has (some) control in which order the variables get prompted.

Any child variable, whose parent variable does not get prompted, shall not be prompted either.

@felangel
Copy link
Owner

felangel commented Apr 22, 2024

Apologies for the delay! I need to think some more about this and do some more research to see if there are any existing standards in place (GitHub workflows conditional steps/jobs comes to mind) but overall the main problems I see with the current proposal are:

  1. It's fragile (not type-safe)
  2. It introduces additional overhead both at compile time and at runtime

Fragile

What I mean by that is, the string "hasAuthentication" has to match in the brick.yaml and in the hooks.

Additional Overhead

In addition, based on the proposal, we'd need to add a new optional file (which needs to be compiled) when you have conditional vars which would result in longer wait times when installing a brick for the first time, and longer execution times when running a brick (on each run).

Summary

For those reasons, I'd prefer to hold off on implementing the proposed solution. Ideally we can arrive at a solution that doesn't have all the same short-comings especially since conditional variable prompting is already possible today and this proposal is focusing on making conditional variables part of the brick.yaml instead of relying on hooks to do the prompting programmatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer:🦄 enhancement candidate Candidate for enhancement but additional research is needed feedback wanted Looking for feedback from the community
Projects
None yet
Development

No branches or pull requests

4 participants