-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement state:modified for saved queries #10295
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10295 +/- ##
==========================================
+ Coverage 88.72% 88.78% +0.05%
==========================================
Files 180 180
Lines 22497 22490 -7
==========================================
+ Hits 19961 19967 +6
+ Misses 2536 2523 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thank you for fixing it 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Two things that are not blocker:
- is it possible to unit test this change?
- should we consider move this
select modified
test to the test for list command and use the newhappy path fixture
to test it? You can create a follow up ticket and give it to me.
Oh and another thing is if we want to list modified semantic model
and metrics, do we need to do similar change? CC @aliceliu since she is working on something related
I think that semantic_models and metrics are already listable. What is Alice working on? |
@ChenyuLInx I'll add some unit tests. As far as a functional test, it's really a selection thing, not precisely list (though they certainly overlap). Will look at what gets into the happy path tests... |
resolves #10294
Problem
Doing state:modified for saved queries didn't work.
Solution
Implement state:modified for saved queries.
Checklist