-
Notifications
You must be signed in to change notification settings - Fork 0
Replace deprecated django-fsm #28
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
Conversation
|
This looks ready for review, so I’ll request one now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall
Nice and tidy dependency-maintenance PR – thanks for picking up the django-fsm deprecation quickly.
The switch to django-fsm-2, together with the Django/Python/boto3 bumps, looks straightforward in the diff. A couple of packaging / CI edges are worth tightening so that the published package and the tested matrix stay in sync.
Specific comments
-
setup.py – Django & boto3 minimum versions
- We still claim
Django>=3.2andboto3>=1.28.57, but CI no longer tests 3.2/4.0/4.1 and requirements.txt now pins boto3 1.39. - Either:
a) keep the broad>=3.2range but re-add the older Django versions to the matrix, or
b) raise the lower bound (e.g.Django>=4.2,boto3>=1.39) to match what we actively support. - I’d lean towards (b) – we’re clearly dropping Python < 3.11 support anyway.
- We still claim
-
Trove classifiers
If we are officially dropping ≤3.10, remember to update theProgramming Language :: Python :: …andFramework :: Django :: …classifiers in setup.py so PyPI reflects reality. -
Version bump
Moving from Python 3.8→3.10 compatibility to 3.11+ and changing a core dependency could be considered a breaking change. Even though we’re still < 1.0, a minor version bump (0.1.0) feels more transparent than 0.0.10 → 0.0.10. -
Import path check
django-fsm-2keeps thedjango_fsmimport path, so our runtime code shouldn’t need touching – but worth a quick grep for anyimport django_fsmvsdjango-fsmtypos before releasing. -
CI matrix
Good to see Django 5.2 in the matrix. Maybe also keep 4.2 so at least one LTS is covered – you already have it, so all good. -
Changelog
Don’t forget to add a note about the dependency drops/bumps – consumers will want to know they need Python ≥ 3.11 and Django ≥ 4.2 (if we go with suggestion 1b).
Summary
Nothing blocking merge, but I’d like us to:
- Align setup.py requirements and classifiers with the new tested range.
- Consider bumping the package version to 0.1.0 and adding a changelog entry.
Once that’s done, happy for this to go in.
|
Most of the earlier feedback has now been addressed (CI matrix and minimum versions updated, dependencies swapped, version bumped). A fresh review will help catch any remaining tidy-ups such as trove classifiers and CHANGELOG entry, so I'll request an auto-review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice quick turnaround—most of the earlier points are now covered 🎉
Remaining nits:
• setup.py still lists the old Python & Django trove classifiers; worth bumping to 3.11/3.12 and 4.2/5.x so PyPI searches are accurate.
• Don’t forget a brief CHANGELOG note for the breaking-change + dependency bumps.
Everything else looks good to me.
Move from
django-fsmtodjango-fsm-2django-fsmis deprecated and there seems to be a good fork that we can just swap like-for-likehttps://github.com/django-commons/django-fsm-2
Also upgrade
python,djangoandboto3