-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Migrate isStructured
method references to MLv2
#37977
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff bb6a4d0...535a442.
|
608de2c
to
0ba4ef7
Compare
0ba4ef7
to
c7ba3ed
Compare
99c809e
to
1511ae8
Compare
1511ae8
to
8d39181
Compare
isStructured
method references to MLv2 [WIP]isStructured
method references to MLv2
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.
Looks good 👍
Left some comments and a question inline.
const isPreviousQuestionNative = | ||
previousQuery && Lib.queryDisplayInfo(previousQuery).isNative; | ||
|
||
if (isNative || isPreviousQuestionNative) { |
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.
Previously, when previousQuestion
was undefined
/null
, !previousQuestion?.isStructured()
evaluated to true
. Now it evaluates to a falsy value. Not sure if it makes things better or worse, but to make it work exactly like before, we should do this:
const isPreviousQuestionNative = | |
previousQuery && Lib.queryDisplayInfo(previousQuery).isNative; | |
if (isNative || isPreviousQuestionNative) { | |
const isPreviousQuestionStructured = | |
previousQuery && !Lib.queryDisplayInfo(previousQuery).isNative; | |
if (isNative || !isPreviousQuestionStructured) { |
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.
I found the original code before @kulyk rewrote it.
https://github.com/metabase/metabase/pull/18827/files#diff-7eae47ba62c31da65c448230eacac88be86470a204d450011acb631d7bb74144L80-L83
So I think we can safely assume that the intention was to make sure the both questions are structured.
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.
Previously, when previousQuestion was undefined/null, !previousQuestion?.isStructured() evaluated to true.
@kamilmielnik I'm 99% sure the older implementation (rewrite) made this assumption by mistake. Do you mind if I leave the code as is?
@@ -494,8 +486,9 @@ class Question { | |||
|
|||
const hasSinglePk = | |||
table?.fields?.filter(field => field.isPK())?.length === 1; | |||
const isStructured = !Lib.queryDisplayInfo(this.query()).isNative; |
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.
In some files we're introducing isStructured
and in some files we're just plainly using !isNative
. I think we should go with 1 approach:
- we can get rid of
isStructured
and always use!isNative
- we can fully embrace
isStructured
by adding it toLib.queryDisplayInfo
result
WDYT?
this is a non-blocking comment
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.
I'm for the first: !isNative
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.
After some weird CI failures that I couldn't reproduce locally, I moved this step to a separate PR.
#38199
b9e8074
to
fdc93b1
Compare
Just as with #38123, this is another place where MLv2 invocation broke because it didn't guard against the internal queries.
fdc93b1
to
535a442
Compare
@nemanjaglumac Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
This is purely semantic change. Please see this comment #37977 (comment)
* Migrate `isStructured` method references to MLv2 * Fix broken audit pages Just as with #38123, this is another place where MLv2 invocation broke because it didn't guard against the internal queries. * Remove `isStructured()` method from the `Question` prototype
This is purely semantic change. Please see this comment #37977 (comment)
* Migrate `isStructured` method references to MLv2 * Fix broken audit pages Just as with #38123, this is another place where MLv2 invocation broke because it didn't guard against the internal queries. * Remove `isStructured()` method from the `Question` prototype
This is purely semantic change. Please see this comment #37977 (comment)
This PR migrates
isStructured()
references to MLv2.It also removes the method altogether from the
Question
prototype.Resolves #37389