Skip to content

Conversation

@josephdpurcell
Copy link

Fixes #116

This PR is a revision of #117. I have to admit I wasn't sure the best way to go about this PR -- let me know if I should instead make it against https://github.com/Sykander/json-logic-js/tree/feature/controlled-execution.

I included @Skyander's commits here. The "diff" from @Skyander's version is that I renamed the param from controlledExecution to traverse, and changed the state tracking object from runOperationAsControlled to opOptions which allows future options to be supported.

Here is a revision to the PR description as well:

Changes:

  • Added traverse option on add_operation function
  • Added unit tests traverse

Description

This PR adds functionality so that when registering a custom operator with the add_operator method, you can now pass in a third param options with the property traverse. This option allows you to specify how your operator should be treated during evaluation and also prevents all nested parameters to the operator from being pre-evaluated.

I believe this PR addresses one of the noted limitations described in https://jsonlogic.com/add_operation.html:

Every other operator including custom operators, performs depth-first recursion.

Once this PR is merged, that sentence could be revised to say:

Every other operator including custom operators, performs depth-first recursion. Unless, the implementation supports a flag to disable the depth-first traversal, such as the JavaScript implementation.

Sykander and others added 3 commits November 17, 2022 14:06
- Added `controlledExecution` option on `add_operation` function
- Added unit tests `controlledExecution`
- Cleaned up first test case
@josephdpurcell
Copy link
Author

If this does get merged, a revision to the Types to include this flag would be useful. I saw DefinitelyTyped/DefinitelyTyped#49234.

@Sykander
Copy link

Sykander commented Feb 2, 2023

Looks good to me 👌

@josephdpurcell
Copy link
Author

Thanks for giving it a review!

I was just talking this week to several engineers about this feature. It's a very powerful concept.

@jwadhams What do you think?

@josephdpurcell
Copy link
Author

Is there any interest in merging this? @jwadhams is there openness to adding a maintainer to help review issues and merge PRs?

@jwadhams
Copy link
Owner

@josephdpurcell hey I made time in my sprint to review this today or tomorrow. What I've read so far I'm really excited about.

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

Successfully merging this pull request may close these issues.

Feature Request: ability to add custom operators which are not automatically traversed for evaluation

3 participants