Add Suggestions for User Input Errors #947
Conversation
metricflow/query/query_parser.py
Outdated
| ) | ||
| lines.append(f"\nError #{issue_counter}:") | ||
| issue_set_lines: List[str] = [ | ||
| "Query Input:\n", |
There was a problem hiding this comment.
Might make sense to put the query input after the error message - I'm guessing users will be looking for the error message before they look back at the query input to debug!
There was a problem hiding this comment.
Also - should the query input go outside the loop? Currently looks like the same UI description will be printed over and over again.
There was a problem hiding this comment.
Swapped the order. With the query input being on the outside, the nesting seemed a little deep,, and given it's unlikely an input would have multiple issues, the reduction in nesting seemed worthwhile.
metricflow/query/query_resolver.py
Outdated
| ) | ||
| # Resolve all where filters in the DAG. | ||
| filter_spec_lookup = self._build_filter_spec_lookup(resolution_dag) | ||
| for filer_spec_resolution in filter_spec_lookup.spec_resolutions: |
There was a problem hiding this comment.
typo: filer_spec_resolution -> filter_spec_resolution
| ) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class VirtualResolverInputForWhereFilterIntersection(MetricFlowQueryResolverInput): |
There was a problem hiding this comment.
Why "Virtual"? Confused what that's supposed to mean in this context 🤔
There was a problem hiding this comment.
NVM, thought about it more and removed that prefix.
There was a problem hiding this comment.
I'm only like halfway through. Hopefully @courtneyholcomb can pick this up and finish the review, but if she doesn't have time I can get to it later this afternoon.
metricflow/query/similarity.py
Outdated
| """From a previous commit: | ||
|
|
||
| Importing fuzzywuzzy causes warning due to non-installed levenshstein package. Suppress this by catching | ||
| the warning during the import in query_parser because this package is not required. | ||
| """ |
There was a problem hiding this comment.
We don't use fuzzywuzzy anymore, I suspect this and the captureWarnings stuff is no longer needed. We can follow up later.
| class WhitelistSpecPatten(SpecPattern): | ||
| """A spec pattern that matches based on a configured whitelist of specs. |
There was a problem hiding this comment.
The new nomenclature for this is AllowList although I prefer to call this one MatchListSpecPattern or similar, it's a bit more descriptive.
|
Thanks for the lack of state notifications there, GitHub..... |
plypaul
force-pushed
the
plypaul--70--agbir-follow-up
branch
from
06f2b0f to
54a9d04
Compare
plypaul
force-pushed
the
plypaul--71--suggestions
branch
from
28c8111 to
d24d18e
Compare
plypaul
force-pushed
the
plypaul--71--suggestions
branch
from
d24d18e to
043c5ac
Compare
Description
This PR adds suggestions to error messages for metrics / group-by-items specified by the user.