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

Make it better compatibility with Django 3.2 #154

Merged
merged 1 commit into from
Dec 16, 2021

Conversation

simkimsia
Copy link
Contributor

@simkimsia simkimsia commented Sep 3, 2021

Description

Include a default auto field in the apps.py as accordance to Django 3.2

Related to #153 as it's also about Django 3.2 compatibility but not the exact same issue

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added an appropriate CHANGELOG entry

@tbrlpld
Copy link
Collaborator

tbrlpld commented Nov 18, 2021

@simkimsia I think the issue you are referencing in the PR description is describing a different deprecation.

Your PR fixes the default auto field: https://docs.djangoproject.com/en/3.2/releases/3.2/#customizing-type-of-auto-created-primary-keys

The issue is about the automatic app config discovery: https://docs.djangoproject.com/en/3.2/releases/3.2/#automatic-appconfig-discovery

Both changes come with Django 3.2 and relate to AppConfig but are distinct.

I would also propose to make the changelog entry more precise and really describe the change you made.

@simkimsia
Copy link
Contributor Author

@tbrlpld i have changed the changelog and repush a new commit. Please check. Thank you

@tbrlpld
Copy link
Collaborator

tbrlpld commented Nov 25, 2021

@simkimsia "Better compatibility" still seems a bit subjective (which I understand is also only my subjective opinion 😅 ah irony).

How about :

Define `AppConfig.default_auto_field` as [required since Django 3.2](https://docs.djangoproject.com/en/3.2/releases/3.2/#customizing-type-of-auto-created-primary-keys).

@simkimsia
Copy link
Contributor Author

I'll change to your suggestion

It sounds better too.

Later tonight I'll push

📝 Rewrite changelog as result

Signed-off-by: KimSia Sim <[email protected]>

📝 UPDATE: changelog to reflect define appconfig field
@simkimsia
Copy link
Contributor Author

@tbrlpld done. pls check

@tbrlpld
Copy link
Collaborator

tbrlpld commented Nov 25, 2021

Looks good to me 👍

@simkimsia
Copy link
Contributor Author

@tbrlpld merge?

@tbrlpld
Copy link
Collaborator

tbrlpld commented Nov 29, 2021

@simkimsia I do not have merge access in the repo. Maybe try mentioning a maintainer.

@simkimsia
Copy link
Contributor Author

@tbrlpld thank you i wasn't aware.

@bcdickinson can help?

@thibaudcolas thibaudcolas added django Related to Django templates capabilities enhancement New feature or request labels Dec 16, 2021
@thibaudcolas
Copy link
Member

Thank you both, great to get this in!

@tbrlpld I’ve added you to the project’s maintainers.

@thibaudcolas thibaudcolas merged commit fae5c33 into torchbox:master Dec 16, 2021
@simkimsia
Copy link
Contributor Author

@thibaudcolas thanks for merging. HOw do i become maintainer? and i noticed one of the compatibility tests failed. should i fix it?

@thibaudcolas
Copy link
Member

@simkimsia at the moment all of the maintainers are Torchbox employees, although if you see yourself using this project and contributing regularly (big or small) I’m sure we’d consider it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
django Related to Django templates capabilities enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants