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
Put an upper bound on bleach and comment each and every dependency #1253
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1253 +/- ##
=======================================
Coverage 79.19% 79.19%
=======================================
Files 109 109
Lines 4657 4657
Branches 531 531
=======================================
Hits 3688 3688
Misses 761 761
Partials 208 208 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM 🥇
And thank you for the comments in the dependencies 💯!
"bleach[css]>=5", | ||
# bleach has been pretty stable, hence the loose pin | ||
"bleach[css]>=5,<6", | ||
# Pillow is used very little and has never broken | ||
"Pillow", |
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.
Shouldn't we also pin the Pillow version?
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.
Could be good, I don't have strong opinions on this one... I don't even know what the lower bound should be.
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.
I'll leave this one as it is, bcz most projects already have/use a pinned Pillow version and Django-Wiki will use that version (if its in the right order).
"django-sekizai>=0.10", | ||
# sorl-thumbnail is maintained by jazzband so it might be | ||
# very stable but it might also suddenly invite breaking changes | ||
"sorl-thumbnail>=12.8,<13", |
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.
Maybe we can someday drop sorl-thubnail and use easy thumbnails? or provide a better integration in order to avoid template tag name collision.
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.
I don't know TBH.
sorl-thumbnail has been quite stable. Not a lot of bugs, but maybe it's good to switch to a project that is actively developed.
@benjaoming Quick question, do we need to draft a new release after this? I believe this is a breaking change and prevents the app to be used. Will wait for your answer in order to merge it. |
Yes, this should be a new release. But IIRC, we already made changes in |
Yeah, I believe this should be the workflow we can follow now:
|
Ah no, that would be a patch version. We should bump to 0.10. This doesn't mean that we'd want to support a 0.9.x series with patch updates, but more to cater for people who expect 0.9.x to not break... because we might break stuff with the series of changes going in here: https://github.com/django-wiki/django-wiki/pulls?q=is%3Apr+is%3Aclosed I did also kind of skip writing change logs 😇 https://github.com/django-wiki/django-wiki/blob/main/docs/release_notes.rst |
Ah oks, that sounds great! can't wait for this release!! |
Using |
Hi @statsconchris I've replied in the discussion #1257 (comment) maybe that option works for now until we release a new version. |
Maybe it's useful to start commenting on why a dependency is there and what expectations we have for the versions that are pinned? What do you think @oscarmcm ? :)
Fixes #1252