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

[Modules] Replace Module->getDataDictionary with Module QueryEngine dictionary. #8260

Merged
merged 6 commits into from
Jan 24, 2023

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Dec 7, 2022

This adds a Module->getQueryEngine() to return the QueryEngine interface for a module. It adds a default NullQueryEngine that implements the interface for modules that have not defined their own interface.

The dictionary module is updated to use the new function call.

Since no modules define the interface yet, testing this probably requires merging it into a branch along with PR #8216 so that there's data to test against in the dictionary. (Assuming you have instruments installed/configured.)

@driusan driusan added Cleanup PR or issue introducing/requiring at least one clean-up operation Blocking PR should be prioritized because it is blocking the progress of another task labels Dec 7, 2022
@laemtl laemtl removed the request for review from jeffersoncasimir December 13, 2022 15:06
@laemtl
Copy link
Contributor

laemtl commented Dec 13, 2022

@jeffersoncasimir Do you have time to review?

@jeffersoncasimir
Copy link
Contributor

jeffersoncasimir commented Dec 20, 2022

@jeffersoncasimir Do you have time to review?

I do not have any instruments set up. I think it may be best for someone else to review it ASAP since it is a blocking PR. Apologies for the delay.

@driusan
Copy link
Collaborator Author

driusan commented Dec 20, 2022

https://github.com/aces/Loris/blob/main/modules/instruments/php/module.class.inc#L163 isn't calling it, it's defining the old function. It's removed in #8216 where the module is updated to provide a full query engine. Looks like you're right about https://github.com/aces/Loris/blob/main/modules/dictionary/php/fields.class.inc#L88. Not sure why phan didn't catch that. Just fixed it.

What are the others? The old getDataDictionary() implementations will stick around (but not be called) for a reference while the modules are updated to provide the full interface in separate PRs.

@driusan
Copy link
Collaborator Author

driusan commented Dec 20, 2022

I deleted the old implementations in the latest commit. The updated instruments query engine will be re-added in PR#8216, the updated candidate_parameters is re-added in PR#8288. The imaging browser one is a simple enough dictionary that it's probably not needed for reference when the module is updated to provide a full query engine.

This adds a Module->getQueryEngine() function to the Module class to
get the QueryEngine to the module. The default is a NullQueryEngine
which does nothing and matches nothing.

Update the dictionary module to use the new interface instead of
Module->getDataDictionary().
@driusan
Copy link
Collaborator Author

driusan commented Jan 24, 2023

@xlecours ping.. I just rebased this because the required tests changed, but it was passing before the rebase.

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

Tested using #8288

@driusan
Copy link
Collaborator Author

driusan commented Jan 24, 2023

Xavier approved, no take-backs!

@driusan driusan merged commit b768f3e into aces:main Jan 24, 2023
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking PR should be prioritized because it is blocking the progress of another task Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants