-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(hogql): errors for large full select queries #15957
Conversation
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.
Works great, but one one I have is that it shouldn't be possible to "Update"/run the query if's got a syntax error.
const [monaco, setMonaco] = useState(null as Monaco | null) | ||
const [editor, setEditor] = useState(null as importedEditor.IStandaloneCodeEditor | null) |
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.
Nit: Since these come as a package together, I propose just one useState
of type [Monaco, importedEditor.IStandaloneCodeEditor] | null
@@ -129,6 +129,7 @@ export const commonConfig = { | |||
'process.env.NODE_ENV': isDev ? '"development"' : '"production"', | |||
}, | |||
loader: { | |||
'.ttf': 'file', |
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.
What's this for?
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.
Got an error when importing from monaco otherwise :/
Will this do? I'm not exposing the "validating..." to the user through a spinner cause I think it'd be flash and annoying. However because of that I'm also not disabling the button when it's loading, and there's a chance someone can still submit an invalid query if they're really fast. But then we show the error below, so 🤷♂️ |
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.
That's great
Problem
The full HogQL insight editor does not show errors as well as the inline filter editors.
Changes
Support passing error information from
HogQLMetadata
now.Also cleaned up the location information with antlr syntax errors.
How did you test this code?
See the gif