Improved handling of user query for higlass items #564
Conversation
|
I tested with properly formatted lists (1-2 items), multi-query strings with commas, single query strings, and empty lists. |
There was a problem hiding this comment.
These functions are pretty messy but without overhauling this whole set of checks and as they mostly work fine not worth the effort to refactor.
I think it's really hard to know what is meant to be entered into the 'search_queries' parameter unless you know that the automated checks have the 'search_queries' stored in the full output and can be used as examples-but you need to know that first.
I was thinking that this initially would be easier to debug if we stored the actual full_query in the full_output (maybe without the field= ... part) but then if you tried to do what I suggested as a way to see what to pass into the parameter it would be even more confusing.
Luckily, I think these get run rarely enough and by those of us who know where to look that again not worth making any changes there.
I do recommend adding another helper function like this:
def get_search_results(connection, check, search_query): try: return ff_utils.search_metadata(search_query, key=connection.ff_keys) except Exception as e: check.full_output.setdefault('search_problems', []).append(e) return []
And then I guess at least for the manual checks we might want to add a bit at the end of each to see if there is 'search_problems' included in the full output and if so set status to FAIL and disable the action?
| check.status = 'PASS' | ||
| check.allow_action = False | ||
| return check | ||
| check, search_queries = verify_queries(check, search_queries, find_opfs_missing_higlass) |
There was a problem hiding this comment.
I think you can simplify the verify_queries function and get rid of the ignore_queries parameter if you move this call into an else after the if at 1172 where if the find_opfs_missing_higlass is passed in it uses the query on the next couple of lines else you can call the verify_queries function.
| @@ -2287,3 +2272,31 @@ def convert_es_timestamp_to_datetime(raw): | |||
| "%Y-%m-%dT%H:%M:%S" | |||
| ) | |||
| return converted_date | |||
|
|
|||
| def verify_queries(check, search_queries, ignore_queries): | |||
There was a problem hiding this comment.
See above - I think you can get rid of the ignore_queries parameter
| if isinstance(search_queries, str): | ||
| # for case where (possibly multiple) query is passed in via kwargs | ||
| queries = search_queries.split(',') | ||
| search_queries = [q.strip() for q in queries] |
There was a problem hiding this comment.
I think it might also be useful to check if the passed query(s) begin with an & and if not add it.
| check.status = 'PASS' | ||
| check.allow_action = False | ||
| return check | ||
| check, search_queries = verify_queries(check, search_queries, False) |
There was a problem hiding this comment.
I don't believe you need to return the check from this function - you are passing a reference to an object which can be modified in the function and that change will be reflected without needing to return it.
Add helper for three functions used by foursight checks querying for higlass items:
find_expsets_processedfiles_requiring_higlass_item,find_expsets_otherprocessedfiles_requiring_higlass_itemsandfind_files_requiring_higlass_items. Queries formatted as strings will be corrected to lists (using input commas to split strings as necessary). A warning will be added tocheck.brief_outputin this case.