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

Nested Directives #27

Open
ptth222 opened this issue Jun 22, 2023 · 6 comments
Open

Nested Directives #27

ptth222 opened this issue Jun 22, 2023 · 6 comments

Comments

@ptth222
Copy link
Collaborator

ptth222 commented Jun 22, 2023

This was mentioned previously in the ISA documentation I sent you and you commented some on it in the PDF. To refresh it's basically allowing directives under some circumstances to call another one to fill in a value. You had some issues with the syntax I proposed.

My recommendation was to indicate the calling of another directive using a forward slash. For example:

 "studies": {
     "no_id_needed": {
         "value_type": "section_matrix",
         "required": "True",
         "headers": [
           "\"description\"=description",
           "\"comments\"=/studies%comments"
         ],
         "table": "study"
         }
     }

The "comments" header would call a directive named "studies%comments". I chose this based on the concept of a JSON Pointer https://json-schema.org/understanding-json-schema/structuring.html#json-pointer. We also need a character to indicate that a directive is nested only and should not be run on its own, and I chose the '%' character since it also makes the names similar to the attributes we have for the extract command. Basically, if a directive has '%' in the name then it is skipped as directives are being looped over.

Are you okay with both of these syntaxes?

@hunter-moseley
Copy link
Member

hunter-moseley commented Jun 23, 2023 via email

@ptth222
Copy link
Collaborator Author

ptth222 commented Jun 23, 2023

In the case of a JSON Pointer it doesn't only refer to subschema. "/" would indicate the whole object, "/properties" would be the value of the properties element. I guess it could be confusing that you are executing the directive rather than replacing with the value, but that seems silly.

I can live with "@" though, or we do "()" at the end and also allow arbitrary variables to be passed in and used. I had mentioned something like this in the documentation, but I decided not to do it for a few reasons. One, the only variables that could be passed in would be record field values and you can already access those with "^" (or "^." if we do that). Two, the error checking and implementation is more complicated. If we don't need or aren't going to use passing in parameters then I don't like "()".

@ptth222
Copy link
Collaborator Author

ptth222 commented Jul 20, 2023

I think we talked about this, but I'm not sure where to put it in the issues we have open. We had talked about making it possible to pass arguments with nested directives, but there is actually a problem that I didn't think about. Most of the examples and use cases for why I created nested directives only have 1 directive in the table, but that isn't required.

Example:

{
# single directive in the table
  "nested_directive1%": {
    "no_id_needed": {
      "execute": "dumb_parse_ontology_annotation(^.entity.id)"
      "value_type": "section"
    }

# multiple directives in the table
  "nested_directive2%": {
    "name1": {
      "override": "asdf"
      "value_type": "str"
    }
    "name2": {
      "override": "qwer"
      "value_type": "str"
    }
  }

In the case of the second example it isn't clear what to do if someone tried to pass in say "override=zxcv".

I see a few options:

  1. Don't allow arguments at all.
  2. Only allow arguments for the single directive situation.
  3. Try to do some sort of smart matching for the multiple directive situation or allow more complicated argument passing.

What do you think?

@hunter-moseley
Copy link
Member

hunter-moseley commented Jul 21, 2023 via email

@ptth222
Copy link
Collaborator Author

ptth222 commented Jul 23, 2023

I have tried to extend the example so maybe it's more clear.

  "nested_directive2%": {
    "name1": {
      "override": "asdf",
      "value_type": "str"
    },
    "name2": {
      "override": "qwer",
      "value_type": "str"
    }
  }

  "directive1": {
    "name3": {
     # Which override field in nested_directive2% gets replaced?
      "fields": ["nested_directive2%(override="zxcv")"],
      "value_type": "str"
    }
  }

I have no specific ideas about the third option. Trying to implement it would change a lot about how things work. Currently if a directive table is called you just execute it, but if you wanted to do some smart arguments you would have to examine the arguments and the fields of the directives within the directive table to see if there was a one to one relationship. Unless you required longer argument names, like 'name1:override="zxcv"', 'name2:override="ljhg"'. You would then either have to require all arguments have this new naming syntax or have special cases where if it doesn't have the directive name then it just matches the first directive that has the field. So 'override="zxcv"' would replace the first or all (all would be easier to implement) directives with an override field, but 'name1:override="zxcv"' would only replace the override field in the "name1" directive.

@ptth222
Copy link
Collaborator Author

ptth222 commented Jul 28, 2023

We discussed this in a meeting and it was decided to have parameter signatures like 'directive_key_name.parameter_name=parameter_value" for specifying parameters to overwrite in specific directives of a directive table. parameter signatures like "parameter_name=parameter_value" will be acceptable, but that parameter name will be replaced in ALL directives inside the directive table that have the parameter. Directives that don't have the parameter defined already will NOT have the parameter inserted.

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

No branches or pull requests

2 participants