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

fix: some Django antipatterns #7867

Merged
merged 7 commits into from Apr 20, 2024
Merged

Conversation

jrief
Copy link
Contributor

@jrief jrief commented Apr 9, 2024

Description

This PR does not change any functionality!

It just fixes two anti-patterns in django-CMS:

force_str(_("…some text…")) is completely useless because it forces to stringify a proxy object. A better and faster way is to use gettext("…some text…") directly.

HttpResponse(json.dumps(content), content_type="application/json") probably is a legacy statement. Django introduced JsonResponse many versions ago.

Related resources

We discussed about this during one of our biweekly meetings.

Checklist

  • I have opened this pull request against develop-4
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

I ran all unit tests successfully on my local environment.

@jrief jrief requested a review from fsbraun April 9, 2024 08:33
@jrief jrief changed the title Fix some antipatterns fix: some Django antipatterns Apr 9, 2024
@jrief
Copy link
Contributor Author

jrief commented Apr 9, 2024

why am I considered as a new contributor and why this notification then fails?

@fsbraun
Copy link
Sponsor Member

fsbraun commented Apr 9, 2024

@jrief I appreciate the initiative! IHMO in the admin files all gettext_lazy can be replaced by gettext, since all of them appear in a request response cycle.

The only question I have: Why does pageadmin need to import IntegrityError?

remove unused import
@jrief
Copy link
Contributor Author

jrief commented Apr 10, 2024

The only question I have: Why does pageadmin need to import IntegrityError?

ups...

some remaining from my private branch

@marksweb
Copy link
Member

Adding my approval to this as I'm passing, pending that bit of cleanup.

@fsbraun fsbraun merged commit c436cf4 into django-cms:develop-4 Apr 20, 2024
80 checks passed
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.

None yet

3 participants