-
-
Notifications
You must be signed in to change notification settings - Fork 182
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: support older versions of colorama #1421
base: master
Are you sure you want to change the base?
Conversation
If we are to accept this fix, #1403 should also be reverted, or maybe the lower bound should just be downgraded to 0.4.4. Removing Colorama entirely would be even better ;) |
Done. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1421 +/- ##
==========================================
- Coverage 97.46% 94.24% -3.23%
==========================================
Files 48 48
Lines 4462 4466 +4
==========================================
- Hits 4349 4209 -140
- Misses 113 257 +144
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
IMHO we shouldn't merge this.
Don't get me wrong. The patch looks quite OK. It's just that older distros will happen always.
Our supported installation methods include pipx and nix, which are both self-contained. You can use also pip and any venv implementation you want.
Thus, I think it's better that our users use self-contained installations, than having to deal with older distros.
At least, that's my opinion right now. But I could be wrong...
Removed modification in poetry.lock and added pragma: nocover
In copier among these installation methods, there is installation method that just makes sure, that there is minimal versions of dependecies that can be used (those dependencies are described in pyproject.toml). I use this installation method and this PR is for those people who use the same method.
Listed distros in my first message are not ancient and are still supported. |
Just trying to understand the motivation for this PR better, @em230418: Why do you need to rely on a Colorama version from an OS distro? Thr |
Because I trust packages from OS distro more, than packages from pypi in general. That thing is not specific for colorama. |
I suppose @em230418 installs Copier like |
I'd vote to entirely remove the call to Colorama. I am myself considering the removal of Colorama from my projects:
Obviously, there are still people using powershell, or cmd.exe through Cmder/else. But modern Windows (10) actually supports ANSI, but it's disabled by default. This SuperUser post shows how to enable it permanently. So we could tell users to do that 🤷 |
Ok for dropping it |
@em230418 do you have intentions to go ahead with this PR and remove colorama? Marking as draft while this isn't ready. |
just_fix_windows_console was introduces in colorama 0.4.6
Some stable GNU/Linux OS use packages with older version of colorama. Examples:
https://packages.altlinux.org/en/p10/binary/python3-module-colorama/noarch/
https://packages.debian.org/bullseye/python3-colorama
In order to use OS specific packages with latest copier (which is required for example here https://github.com/OCA/oca-addons-repo-template/blob/master/copier.yml#L4), this patch is introduced.