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(insights): edit as sql #18055

Merged
merged 4 commits into from
Oct 18, 2023
Merged

feat(insights): edit as sql #18055

merged 4 commits into from
Oct 18, 2023

Conversation

mariusandra
Copy link
Collaborator

Problem

The lifecycle insight is missing the "edit as sql" button change from this large PR.

Changes

2023-10-18 11 45 50

  • The unsaved "new insight" view now also gets a "..." menu, which exposes the "view source" / "hide source" button.
  • Adds a "Edit SQL directly" option to the menu, in case the insight's response contains a hogql field.
  • The flow for opening a new insight is a bit ugh, but I'd like to save it for a follow up PR. The ugh: this works on the "edit insight" or "show insight" pages, but is only ugh on the "new insight" page. When you click "edit sql directly", some logic mess is causing the "lifecycle" tab to remain open. I'm opening the canonical new insight URL, but it's not working as intended. Related: we should talk about putting the query inside the URL's hash params, as the back button otherwise won't really take you back to the previous insight... as the URLs are the same.
  • Note: this works if you click "edit sql" on a saved insight.
  • Note 2: the lifecycle insight is feature flagged, so nothing is public now anyway.

How did you test this code?

Clicked around in the interface.

@robbie-c
Copy link
Member

It'd be great if the SQL was pretty-printed in some way - is there a way of doing that easily? There's clickhouse-format, not sure if we can use that from python

@mariusandra
Copy link
Collaborator Author

That would be great, but the only real solution is to implement pretty printing ourselves in printer.py. I tried all the nodejs-based SQL pretty pretty printers, and they trip over the combination of clickhouse's syntax + our modifications. If the formatter didn't support ClickHouse, then all the other syntaxes failed at some point (e.g. lambdas? SETTINGS clause, etc). If the formatter did, then it still didn't like the $ symbol in property names. I didn't try python libraries, but considering how unmaintained most of the ecosystem is, I'm afraid to look.

In any case, the "correct" solution isn't that hard. We're already printing HogQL within printer.py. We just need to add some code to look at printed line widths, and add whitespaces where relevant. I'm sure we could get a "good enough" printer with little effort. However... that's still too much effort for this low effort PR :D

@mariusandra mariusandra merged commit 3eecbba into master Oct 18, 2023
74 checks passed
@mariusandra mariusandra deleted the lifecycle-hogql-edit branch October 18, 2023 10:20
daibhin added a commit that referenced this pull request Oct 23, 2023
Justicea83 pushed a commit to Justicea83/posthog that referenced this pull request Oct 25, 2023
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.

2 participants