-
Notifications
You must be signed in to change notification settings - Fork 530
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
Reduce database queries on dashboards #3561
Conversation
I'm getting this error on the locale pages:
|
Should be fixed now, was a silly mistake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Tested locally and on stage and performance seems better. I also didn't spot any issues with the stats and latest activity data.
Left some comments inline.
cast( | ||
BaseManager[Project], Project.objects.visible_for(request.user).available() | ||
), | ||
slug=slug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast()
is a no-op at runtime, but with it the type of project
is correct within this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate the intent behind improving type safety, our Python codebase does not use static typing consistently. Introducing it in just this one place creates an inconsistency that might confuse other developers who are not expecting type hints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, type hints were how I was able to catch the bug fixed in #3562. Without them, working with Pontoon code is much more difficult and more error-prone, in particular for developers not previously familiar with it.
We should be working towards more type hinting being available in Pontoon.
I got |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deployed latest version to stage.
cast( | ||
BaseManager[Project], Project.objects.visible_for(request.user).available() | ||
), | ||
slug=slug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate the intent behind improving type safety, our Python codebase does not use static typing consistently. Introducing it in just this one place creates an inconsistency that might confuse other developers who are not expecting type hints.
Co-authored-by: Matjaž Horvat <[email protected]>
@flodolo Could you take this for a spin on stage? |
Seems to work fine for me, couldn't find inconsistencies. |
Fixes #3545
This should speed things up a bit. Two related changes are included to resolve the identified inefficiencies, and a couple of the
Locale
methods are dropped/simplified to match current usage. TheProjectLocale.get_latest_activity()
function is split into two separate implementations onLocale
andProject
, as they are now rather different, and theget_object_or_none()
utility is dropped as obsolete.