Improvements for the I_DEFINITION_QUERY interface
Description
Activity
Pablo Pazos February 26, 2019 at 4:56 AM
Checked the commit and changes that were done look OK!
REF: https://github.com/openEHR/specifications-SM/commit/3da77e559887a52a61bf1f2f4c2e38520c09a608
Pablo Pazos February 13, 2019 at 7:47 PM
Thanks Thomas, please let me know when those are committed to https://github.com/openEHR/specifications-SM/commits/master (I believe that is the spec) to check the diffs.
About 2. my rationale was:
Try to think of SM operations semantically not technically. In that sense, the list_xxxxxx are all the same operation looking for the same goal, just have different technical requirements.
In my implementations, I use a pattern that avoids code duplication, that is for the “list” operations to have optional matchers or filter parameters, to narrow down the list. But the operation is the same. The operation without parameters are defined to use the “match all” or “not filter” default values.
IMO there are no tricks when everything is well documented.
This of course is a matter of design approaches, those ^ are my “pro” arguments. I’m not sure if I have a “con” argument for my proposal, maybe that the name is not explicit on the operation, but again: the semantics of list_matching(matcher) and list(matcher) are the same, and with the correspondent documentation I don’t see any issues for readers and developers.
On the other hand, having the criteria of adding the behavior on the operation name, if we want to be consistent, we need to add that to every operation that adds some functionality over another functionality like list_matches() adds the matches to the basic list() operation, which I think is adding complexity to the APIs without gaining to much value.
For 4. I think query sets were discussed in a SEC meeting. IIRC we agreed that some queries might be defined or used together, for instance, to populate a GUI or to be used when evaluating a rule. I think that should be added and act, for now, just as a container with an ID and maybe a name. Then we can use this first approach to define it further. So instead of removing it, why don’t we mark it as experimental or trial on the spec?
Thomas Beale February 13, 2019 at 3:07 PMEdited
I have done 1., also for the I_DEFINITION_xx interfaces, since the same logic applies.
I'm not sure I agree on 2. yet, since I think functions that don't have 'trick' parameters to get certain results are not good; however, we could easily make those xx_pattern args optional, and defined Void to mean 'get all'. For discussion; for now, I've left both functions intact.
3. Agree. I've made changes there - row_offset -> item_offset; rows_to_fetch -> items_to_fetch. If we want to redesign this to some concept of 'pages', we'll need a discussion, or at least I'll need a clear model of what you prefer here.
4. Probably true, but we don't know the design of query sets yet; I suspect I should take this out, or even create an I_DEFINITION_QUERY_SET interface for them. Needs more discussion I think.
REF: https://specifications.openehr.org/releases/SM/latest/openehr_platform.html#_i_definition_query_interface
Some suggestions for areas that can be improved:
1. Instead of list_all_queries(), name that function list_queries(). "all" implies something about the result that is not true, since the results will be paginated most of the time wont return "all" the existing queries.
2. Merge list_all_queries() and list_matching_queries() functions, having optional matcher parameters (id_pattern and artefact_id_pattern) on the list_queries function. Since the functionality with list_matching_queries(., ., n, m) is the same as list_all_queries(n,m).
3. For list_xxxx change the names of the pagination parameters from "row"offset" and "rows_to_fetch" to something like offset/max or offset/page_number. The term "row" seems to imply a specific implementation. More generic names, and specific to the feature they provide, "pagination" in this case, are better IMO.
4. If there is a register_query_set() function, there should be also a get_query_set(), and maybe, execute_query_set(). I believe query sets will be registered with some purpose to bring many results that might be, for instance displayed together on the same GUI, for that an execute for all the queries on the query set might be useful to avoid running each query individually from the client.
These suggestions are related, but can be broken in many smaller tickets.