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

Adapt __str__ and __repr__ methods for DB #11329

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented May 16, 2024

This commit removes some debugging functionability in favor of production DB. __str__ and __repr__ methods won't be so descriptive now since we are removing some information from their rendering. This is because to render them properly we need to hit the DB multiple times in the worst case --generating 500 on some user requests that need to be logged in Sentry/New Relic.

There are better ways for this: disabling logging on production and enabling it on DEBUG + Django Shell, but that requires more extra work that doesn't seems super priority right now. We can come back later and add them as we need them if we want.

Closes #10954

This commit removes some debugging functionability in favor of production DB.
`__str__` and `__repr__` methods won't be so descriptive now since we are
removing some information from their rendering. This is because to render them
properly we need to hit the DB multiple times in the worst case --generating 500
on some user requests that need to be logged in Sentry/New Relic.

There are better ways for this: disabling logging on production and enabling it
on DEBUG + Django Shell, but that requires more extra work that doesn't seems
super priority right now. We can come back later and add them as we need them if
we want.

Closes #10954
@humitos humitos requested a review from agjohnson May 16, 2024 08:45
@humitos humitos requested a review from a team as a code owner May 16, 2024 08:45
@humitos humitos requested a review from stsewd May 16, 2024 08:45
@kingcb11

This comment was marked as spam.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This is probably all safe, but I did want to note that we have used the model's string representation in some of the UIs in templates. It would be worth a pass through the various admin pages for any signs of this. No particular model comes to mind at least though, so this is mostly safe I'm sure.

@humitos
Copy link
Member Author

humitos commented May 21, 2024

It seems there are some footer tests failing because of this.

-  'project': 'Version 0.8.1 of Pip (19)',
+  'project': '0.8.1',

This comes from

"project": "Version 0.8.1 of Pip (19)",

However, I think this is not super important because this is the footer API and we are not using the version_compare.project field. Besides, this API will be eventually replaced by addons completely.

@humitos
Copy link
Member Author

humitos commented May 21, 2024

By the way, the version_compare.project does not return the project but the str representation of the version, which is wrong anyways 😄 . I won't fix that since we are not using it, but just wanted to mention it here.

@humitos humitos enabled auto-merge (squash) May 21, 2024 10:09
@humitos humitos merged commit ba32bd2 into main May 21, 2024
4 checks passed
@humitos humitos deleted the humitos/improve-str-repr-for-db branch May 21, 2024 10:43
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.

Template errors result in long development/production response times
3 participants