10000 fix: some Django antipatterns by jrief · Pull Request #7867 · django-cms/django-cms · GitHub
[go: up one dir, main page]
Skip to content

fix: some Django antipatterns #7867

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 20, 2024
Merged

fix: some Django antipatterns #7867

merged 7 commits into from
Apr 20, 2024

Conversation

jrief
Copy link
Contributor
@jrief jrief commented Apr 9, 2024 •
edited
Loading

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
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?

@jrief
Update pageadmin.py
b994119 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marksweb marksweb marksweb approved these changes

@fsbraun fsbraun fsbraun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrief @fsbraun @marksweb
0