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

chore: Upgrade pydantic, mypy, and others #19660

Merged
merged 23 commits into from
Jan 10, 2024
Merged

Conversation

webjunkie
Copy link
Contributor

@webjunkie webjunkie commented Jan 9, 2024

Problem

Upgrading pydantic and drf-spectacular so they work together.
Preparing for other PRs, i.e. #19408

Changes

How did you test this code?

  • relying on tests

Copy link
Contributor

github-actions bot commented Jan 9, 2024

Size Change: -2.65 kB (0%)

Total Size: 2 MB

Filename Size Change
frontend/dist/toolbar.js 2 MB -2.65 kB (0%)

compressed-size-action

@webjunkie webjunkie requested a review from pauldambra January 9, 2024 14:41
@webjunkie
Copy link
Contributor Author

@pauldambra I saw that you tried to update mypy before. I need to update it here, because updating pydantic and others breaks it (as noted above). I found mypy-baseline as a tool to work around the issue that the update causes 1229 problems 😅
Do you think this is a way to go?

@webjunkie webjunkie changed the title chore: Upgrade pydantic and related chore: Upgrade pydantic, mypy, and others Jan 9, 2024
@pauldambra
Copy link
Member

@webjunkie nice! That seems like a good trade-off... we're effectively ignoring those errors by not upgrading mypy... I like that the tool prompts you on progress too so it's that little bit harder to forget 😊

@@ -157,141 +158,5 @@
prop
HAVING steps = max_steps)
GROUP BY prop
'
---
# name: ClickhouseTestExperimentSecondaryResults.test_basic_secondary_metric_results.5
Copy link
Member

Choose a reason for hiding this comment

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

i think this is normally snapshots that no longer exist...

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

scanned 59 of 97 files

some changes to the snapshots (although nothing automatically worrying)

looking at the changelog for syrupy there's

  • numerically sort snapshots if possible

which might explain the changes

@@ -286,124 +173,10 @@
prop
HAVING steps = max_steps)
GROUP BY prop
'
---
# name: ClickhouseTestFunnelExperimentResults.test_experiment_flow_with_event_results_and_events_out_of_time_range_timezones.2
Copy link
Member

Choose a reason for hiding this comment

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

same

@@ -279,88 +280,10 @@
AND NOT "posthog_annotation"."deleted")
ORDER BY "posthog_annotation"."date_marker" DESC
LIMIT 1000 /*controller='project_annotations-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/annotations/%3F%24'*/
'
---
# name: TestAnnotation.test_retrieving_annotation_is_not_n_plus_1.15
Copy link
Member

Choose a reason for hiding this comment

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

same

@@ -1538,28 +1539,10 @@
WHERE (NOT ("posthog_dashboard"."deleted")
AND "posthog_dashboard"."id" = 2)
LIMIT 21 /*controller='project_insights-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/insights/%3F%24'*/
'
---
# name: TestInsight.test_listing_insights_does_not_nplus1.30
Copy link
Member

Choose a reason for hiding this comment

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

and again

SELECT "posthog_instancesetting"."id",
"posthog_instancesetting"."key",
"posthog_instancesetting"."raw_value"
FROM "posthog_instancesetting"
WHERE "posthog_instancesetting"."key" = 'constance:posthog:PERSON_ON_EVENTS_ENABLED'
ORDER BY "posthog_instancesetting"."id" ASC
LIMIT 1 /*controller='project_dashboards-detail',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/dashboards/%28%3FP%3Cpk%3E%5B%5E/.%5D%2B%29/%3F%24'*/
'
---
# name: TestDashboard.test_loading_individual_dashboard_does_not_prefetch_all_possible_tiles.315
Copy link
Member

Choose a reason for hiding this comment

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

same

@@ -364,66 +357,21 @@
FROM "posthog_organizationmembership"
INNER JOIN "posthog_organization" ON ("posthog_organizationmembership"."organization_id" = "posthog_organization"."id")
WHERE "posthog_organizationmembership"."user_id" = 2 /*controller='project_notebooks-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/notebooks/%3F%24'*/
'
---
# name: TestNotebooks.test_updates_notebook.20
Copy link
Member

Choose a reason for hiding this comment

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

same 🫠

@webjunkie webjunkie merged commit 7cabe21 into master Jan 10, 2024
93 checks passed
@webjunkie webjunkie deleted the chore/upgrade-deps-pydantic branch January 10, 2024 08:32
jacobwgillespie pushed a commit to jacobwgillespie/posthog that referenced this pull request Jan 12, 2024
* Upgrade pydantic and all related
* Upgrade mypy
* Add mypy-baseline

To update baseline when you fix something (only then!) use:

    [mypy cmd] | mypy-baseline sync
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