Skip to content

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 Category: Cleanup PR or issue introducing/requiring at least one clean-up operation State: 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.

@xlecours
Copy link
Contributor

@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

Category: Cleanup PR or issue introducing/requiring at least one clean-up operation State: Blocking PR should be prioritized because it is blocking the progress of another task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants