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

fix for custom builtins not being used when parsing rules #1268

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

mattmarshall
Copy link
Contributor

@mattmarshall mattmarshall commented Apr 21, 2022

Rule.rulesFromURL(String, BuiltinRegistry) does not pass the BuiltinRegistry argument to Rule.rulesParserFromReader(). This means that a user-specified builtin registry cannot be used with rules that are loaded using rulesFromURL(). This seems like a bug, so I can create a corresponding GitHub or JIRA issue, but wanted to first present this pull request. This is a simple one-line fix to pass registry into rulesParserFromReader(), along with some additional unit tests and a minor reorganization for some of the .rules files.

If you checkout just the TestRuleLoader.java unit test file from this PR, you can see that the load_from_file_with_custom_builtins() test fails. After adding the change to Rule.java, the test passes.

@afs
Copy link
Member

afs commented Apr 22, 2022

Hi @mattmarshall, thanks for the PR.

Yes, please do create a GH issue or a JIRA.

@afs
Copy link
Member

afs commented Apr 28, 2022

Looks good and the build is passing.

@der -- unless I heard otherwise, I'll merge this.

@afs afs self-requested a review April 28, 2022 10:03
@der
Copy link
Contributor

der commented Apr 28, 2022

Yes, looks good to me. Thanks @mattmarshall

@afs afs merged commit f456247 into apache:main Apr 28, 2022
@mattmarshall
Copy link
Contributor Author

Thanks for merging this @afs and @der. I hadn't gotten around to creating an issue for this yet, so I appreciate it!

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.

3 participants