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

Use events model table instead of sql formatting #5303

Closed
wants to merge 1 commit into from

Conversation

fuziontech
Copy link
Member

Changes

WIP - Makes things easy

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • New/changed UI is decent on smartphones (viewport width around 360px)

@timgl timgl temporarily deployed to posthog-pr-5303 July 23, 2021 00:31 Inactive
@macobo
Copy link
Contributor

macobo commented Jul 23, 2021

This ties us deeper with the infi ORM - given

  1. We're not using it in any of our queries (and we shouldn't since ORMs suck for our purposes)
  2. And the ORM is of questionable quality

This seems like a net loss rather than making anything better. Leaky abstractions and all that.

Although you might be going somewhere else here given the WIP nature of this.

@neilkakkar
Copy link
Collaborator

.. and the ORM hasn't seen activity in a while 😅 .

Side question: Why do we use the ORM for Clickhouse migrations?

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to 2 weeks of inactivity. Feel free to reopen it if still relevant.

@posthog-bot posthog-bot closed this Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants