Skip to content
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(hog): refactor code editors, hog validation #23054

Merged
merged 57 commits into from
Jun 24, 2024
Merged

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Jun 18, 2024

Changes

  • Takes all HogQL validation code out of HogQLQueryEditor and moves it into CodeEditor. The reason is that HogQLQueryEditor exploded in scope, now containing data warehouse and AI features we don't need.
  • Adds codeEditorLogic that takes care of errors and validation based on the language
  • Adds program to the HogQLMetadata query and uses it to validate Hog programs
  • Refactor a few NotImplementedErrors (not exposed) into QueryError/SyntaxError (exposed to users)

Not yet here (coming soon):

  • Something is broken with syntax errors: we don't get the right line numbers from the C++ parser. However this is equally broken for HogQL as well, so I will fix, but might do it outside of this PR to keep things separate.
  • Autocomplete for Hog functions/variables is not implemented.
  • Inline HogQL editors for small fields and template strings are not yet implemented.

How did you test this code?

  • Edited Hog and HogQL code in the browser, saw errors show up.

Show don't tell

  • With the python parser (syntax errors are parsed correctly, is slow)
    2024-06-21 11 29 38

  • With the CPP parser (syntax errors are not parsed, is fast)
    2024-06-21 11 31 11

@mariusandra mariusandra changed the base branch from master to hog-plus-plus June 20, 2024 13:50
@mariusandra mariusandra mentioned this pull request Jun 20, 2024
55 tasks
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (wasn't pushed!)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Base automatically changed from hog-plus-plus to master June 21, 2024 16:22
@mariusandra mariusandra merged commit 5ab1077 into master Jun 24, 2024
91 checks passed
@mariusandra mariusandra deleted the hogql-autocomplete branch June 24, 2024 07:59
],
})),
selectors({
isValidView: [(s) => [s.metadata], (metadata) => !!(metadata && metadata[1]?.isValidView)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metadata isn't updating now -> hogQLQueryEditorLogic connection to this field isn't updating either so this value is always false. Trying to debug right now but noting for visibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants