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

Changes to allow a migration of DRF tokens (#220 and #215) #295

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gawry
Copy link

@gawry gawry commented Jan 11, 2023

No description provided.

@giovannicimolin
Copy link
Contributor

giovannicimolin commented Oct 12, 2023

@gawry Thanks for the contribution! 😁

After reviewing your changes, I don't think the best place for this operation is a migration. The changes explicitly assume that people are using DRF's authtoken model, which isn't always the case.
With interest in keeping this library as agnostic to external libraries as possible, I'm inclined to reject these changes.

That said, there's some useful content in this PR that can become a nice piece of documentation guiding new users on how to perform the token migration on their applications. Are you interested in changing this PR to a documentation improvement?


Edit: I'll try to find some time to reply to the issues you linked in the next few weeks.

@gawry
Copy link
Author

gawry commented Oct 16, 2023

Hello @giovannicimolin,

Thank you for taking the time to review my PR.

I understand the concern about keeping the library agnostic to external libraries. The intention behind my changes was to ensure that the operation would only be executed when the user is leveraging DRF's authtoken model. If they aren't using it, the migration would simply be a no-op and wouldn't have any impact.

By integrating this feature, we can provide a seamless experience for those who are using the authtoken model, without affecting those who aren't. The design has been crafted with the very purpose of ensuring that the library remains flexible and agnostic.

However, if you still believe that this shouldn't be part of the core migration, I'm more than happy to pivot this PR into something else. Perhaps a documentation piece or a command. This could guide users on how to handle token migration if they're using the authtoken model, as you suggested.

Looking forward to your feedback.

Best,
Gawry

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