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

Misc fixes #509

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Misc fixes #509

wants to merge 9 commits into from

Conversation

lebaudantoine
Copy link
Collaborator

@lebaudantoine lebaudantoine commented Dec 16, 2024

Purpose

Fix misc small issues (i.e. typos, warnings, etc.)

Proposal

Mostly nit-picking. Please check my commits.

Few warnings are still raised.
One concerns boto, but few PRs address it, boto/botocore#3239.

Will squash commits if you feel it's necessary

Fix a deprecation warning from Django, which appears while running
tests. 'check' argument is replaced by 'condition'.
_after_postgeneration method will stop saving the instance after
postgeneration hooks in the next major release.

Solved using Claude, feel free to challenge my fix.
Found this solution googling on Stack Overflow.
Without a default ordering on a model, Django raises a warning, that
pagination may yield inconsistent results.

Please feel free to challenge my fix.
Resolved RemovedInDjango60Warning by ensuring format_html() is called
with required arguments, addressing compatibility with Django 6.0.

/!\ Fix by
Claude, need real-world testing. Linterand tests pass.
output seems to be redefined few lines after.
Please feel free to challenge this change.
Found typos while working on the project using my IDE, fixed them.
Sorry for the big commit.

Not a big deal, can totally drop this commit.
These parentheses seem useless to me, remove them.
Please challenge this commit.
I feel these imports aren't used in this migration file.
@lebaudantoine lebaudantoine changed the title [WIP] misc fixe [WIP] misc fixes Dec 16, 2024
`sentry_sdk.configure_scope` is deprecated and will
be removed in the next major version.

(commit taken from people by @qbey)
@lebaudantoine lebaudantoine marked this pull request as ready for review December 31, 2024 10:25
@lebaudantoine lebaudantoine changed the title [WIP] misc fixes Misc fixes Dec 31, 2024
@@ -619,8 +619,9 @@ def post_setup(cls):
release=get_release(),
integrations=[DjangoIntegration()],
)
with sentry_sdk.configure_scope() as scope:
scope.set_extra("application", "backend")
# Add the application name to the Sentry scope
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Add the application name to the Sentry scope
# Add "application" and "backend" tags the Sentry scope

Copy link
Member

Choose a reason for hiding this comment

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

should it be : scope.set_tag("application", "backend")
application was opposed to "worker" for celery if I remember well

Copy link
Member

Choose a reason for hiding this comment

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

I’ve since noticed that the API has changed to deprecate set_extra in favor of set_context,

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