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

Feature - input / output rulemaps for linked rules #23

Merged
merged 5 commits into from
Jul 24, 2024

Conversation

brysonjbest
Copy link
Collaborator

@brysonjbest brysonjbest commented Jul 23, 2024

  • Linked rules recursively compile all inputs and outputs into the top-level rule
  • Update to fix bug on handling of csv generation for comma escapes

@brysonjbest brysonjbest requested a review from timwekkenbc July 23, 2024 22:53
Copy link
Collaborator

@timwekkenbc timwekkenbc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good. This stuff just continues to get more and more annoyingly convoluted though - that's not a comment about your implementation though, just about the difficulty of approaching the problem this way. Alternative approaches such as where the rule author defines the input schema look more and more appealing.

Comment on lines 67 to 71
const uniqueFieldsMap = new Map<string, any>();

fields.forEach((field) => {
uniqueFieldsMap.set(field.property, field);
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just use map here: const uniqueFieldsMap = new Map(fields.map(field => [field.property, field]));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated! Definitely cleaner, thanks :)

Comment on lines 149 to 150
{ key: 'expr1', value: 'field3' },
{ key: 'expr2', value: 'complexExpr + 2' },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, thanks for these tests as I was wondering what the difference between a simple vs complex expression looked like. Second, I think the first one here is still a simple one though, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated so they are both complex. Initially had only one as then you could compare in the same test and see that it worked correctly while not impacting non-complex expressions, but it's sort of redundant with the simple expressions test so revised per your comment.

@brysonjbest brysonjbest merged commit e2c41fc into dev Jul 24, 2024
2 checks passed
@brysonjbest brysonjbest deleted the feature/nested-rulemaps branch December 19, 2024 23:24
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.

2 participants