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

Type trend logic #3152

Merged
merged 12 commits into from
Feb 3, 2021
Merged

Type trend logic #3152

merged 12 commits into from
Feb 3, 2021

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented Feb 1, 2021

Changes

Please describe.
If this affects the frontend, include screenshots.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@EDsCODE EDsCODE temporarily deployed to posthog-type-trend-logi-ubjllw February 1, 2021 19:43 Inactive
@EDsCODE EDsCODE had a problem deploying to posthog-type-trend-logi-ubjllw February 1, 2021 20:02 Failure
@EDsCODE EDsCODE had a problem deploying to posthog-type-trend-logi-ubjllw February 1, 2021 20:15 Failure
@EDsCODE EDsCODE temporarily deployed to posthog-type-trend-logi-ubjllw February 1, 2021 20:35 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-type-trend-logi-ubjllw February 2, 2021 01:11 Inactive
@EDsCODE EDsCODE marked this pull request as ready for review February 2, 2021 01:40
@EDsCODE EDsCODE requested a review from macobo February 2, 2021 15:22
@EDsCODE EDsCODE temporarily deployed to posthog-type-trend-logi-ubjllw February 2, 2021 15:22 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-type-trend-logi-ubjllw February 2, 2021 16:31 Inactive
breakdown_value,
people.next
)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

since we're properly type checking now, what should we do if listeners do nothing here? maybe emit an error so we can log it? and show UI error

Copy link
Contributor

@macobo macobo Feb 3, 2021

Choose a reason for hiding this comment

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

Not sure what you mean - if they do nothing in what sense?

You mean if values.people is null?

If we think a state is impossible honestly an type assertion is ok here. e.g. const people = values.people as something or const { people: currPeople, count, action, label, day, breakdown_value, next } = values.people! though we have prohibited the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah ok we can roll with that

@EDsCODE EDsCODE temporarily deployed to posthog-type-trend-logi-ubjllw February 3, 2021 19:12 Inactive
@EDsCODE EDsCODE merged commit d0318cc into master Feb 3, 2021
@EDsCODE EDsCODE deleted the type-trend-logic branch February 3, 2021 19:26
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