I can agree that the current set of exceptions that we raise in those query methods (and many other methods) is hard to use to identify specific issues. What we have today is a mix of LookupError, KeyError, RuntimeError, ValueError and its subclasses; to make exceptions more useful we probably need more specific and more structured class hierarchy for exceptions. It's a non-trivial problem, of course, to try to update existing code to generate different exceptions, but I can try something reasonably simple on this ticket.
From the client perspective, I think it's probably worth to try to distinguish following cases for these three query methods:
- Unknown keyword argument name (e.g. querySomething(..., not_a_thing="thing")), today this raises KeyError, maybe worth defining something DimensionNameError? (Same for the key in dataId argument).
- Unexpected value for a dimension passed in dataId or keyword arguments, LookupError is raised today, maybe raise new exception InvalidDataIdError?
- For inconsistent/conflicting coordinate values we already raise InconsistentDataIdError, which inherits ValueError, maybe make a new base class for it common with InvalidDataIdError?
- Bad syntax in user expression today raises RuntimeError, replace that with more specific UserExpressionSyntaxError?
- Valid user expression syntax, but containing unknown governor dimension values (e.g. "instrument = 'LSTS'"), with the changes on this ticket raises LookupError, could replace it with the UserExpressionInvalidDataIdError (maybe a subclass of InvalidDataIdError)?
- In other cases methods can return empty result, and explain_no_results() can be used to try to identify the issue
This may not cover all possible errors though, other types of exceptions may leak from the code we have no control over, e.g. database connection may break at any moment and produce some SQLAlchemy errors. We could catch them all at registry boundary and re-throw as a generic exception type (RegistryError), or we can leave that to the caller. I believe general good practice in the cases like this is to catch and wrap/re-throw all exceptions at the module/library boundary and not let undocumented exceptions leak.