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: Use TypeVar for type-hinting of the user parameter #1496

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

payamnj
Copy link
Contributor

@payamnj payamnj commented Sep 13, 2024

Description

Using the AbstractBaseUser for type-hinting the auth user parameter to fix the problem mentioned in issue #1495

Related resources

  • #...
  • #...

Checklist

  • I have opened this pull request against master
  • 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.

@fsbraun
Copy link
Member

fsbraun commented Sep 13, 2024

Thank you very much for this PR!

Quick question for my understanding: Why did you not use TypeVar as you suggested in #1495 ?

@payamnj
Copy link
Contributor Author

payamnj commented Sep 13, 2024

I wanted to know if we can be more specific, and Django recommendation for custom auth user model is to inherit from AbstractBaseUser, AbstractBaseUser so I thought it should work for the majority of the cases.

But since it's just a recommendation, maybe I should use the TypeVar and allow more flexibility? What do you think?

@fsbraun
Copy link
Member

fsbraun commented Sep 14, 2024

Maybe with a TypeVar we have the most flexibility?

@payamnj
Copy link
Contributor Author

payamnj commented Sep 15, 2024

@fsbraun How about using models.Model instead? Any custom AUTH_USER_MODEL must be a subclass of Django Model, so this will provide more accurate type hinting and improve editor support.

@fsbraun
Copy link
Member

fsbraun commented Sep 15, 2024

@payamnj That sounds good to me! Maybe with a comment that a user model is expected but not imported...!

@payamnj payamnj changed the title fix: Use AbstractBaseUser for type-hinting of the user parameter fix: Use TypeVar for type-hinting of the user parameter Sep 15, 2024
Copy link
Member

@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

Thanks! Great work!

@payamnj
Copy link
Contributor Author

payamnj commented Sep 17, 2024

@fsbraun can we merge this change and publish a new version?

@fsbraun fsbraun merged commit ff2602c into django-cms:master Sep 17, 2024
34 checks passed
@fsbraun
Copy link
Member

fsbraun commented Sep 18, 2024

@payamnj Version 3.2.3 is on its way! Have you joined the django CMS discord community yet? https://discord-main-channel.django-cms.org/ There's a #django-filer channel, too.

@payamnj
Copy link
Contributor Author

payamnj commented Sep 18, 2024

@fsbraun No I have not yet, I will join. Thank you 🙏

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