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
pythonPackages.{hiredis,aioredis,channels_redis,django-crispy-forms,django-filter}: init; Added baddecisionsalex to maintainers. #63545
Conversation
We have an eval failure
Looks like a entry was removed from the maintainers list in 59446b9 |
Hey I was pretty swamped the last few days. I’ll fix the maintainers list. Since it was the first commit is it possible to rebase it back there without needing to redo all of the later ones? |
There's no hurry. Probably the least confusing way of editing underlying commits when they're quite well separated is to add (again, separate) commits on top of the existing ones with temporary commit comments (e.g. |
807e98c
to
bf09c3b
Compare
Okay I got the pulled in the maintainer's list from master and modified that old commit. I don't know if "adding" the changes from master is problematic. If it is let me know and I can re-edit the commits. I am just seeing the rest of these conversations about other fixes. I will work my way through those now. |
bf09c3b
to
551da68
Compare
7aa26df
to
85c2dac
Compare
@FRidh I think that might have wrapped this one up. |
I think you might need to take another stab at the "add to maintainers" commit, making it only add yourself. |
( |
85c2dac
to
e2c79ad
Compare
Hey I finally got that fixed :) |
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.
Left some suggestions that, when implemented, should be acceptable for merge 👍
I'd recommend reading the suggestions commit by commit.
meta = with stdenv.lib; { | ||
homepage = "http://github.com/django/channels_redis"; | ||
license = licenses.mit; | ||
description = "asyncio (PEP 3156) Redis client library."; |
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.
description = "asyncio (PEP 3156) Redis client library."; | |
description = "asyncio (PEP 3156) Redis client library"; |
meta = with stdenv.lib; { | ||
homepage = "https://github.com/carltongibson/django-filter/tree/master"; | ||
license = licenses.bsdOriginal; | ||
description = "Django-filter is a reusable Django application for allowing users to filter querysets dynamically."; |
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.
description = "Django-filter is a reusable Django application for allowing users to filter querysets dynamically."; | |
description = "Reusable Django application for allowing users to filter querysets dynamically"; |
meta = with stdenv.lib; { | ||
homepage = "http://github.com/redis/hiredis-py"; | ||
license = licenses.bsdOriginal; | ||
description = "Python extension that wraps protocol parsing code in hiredis. It primarily speeds up parsing of multi bulk replies."; |
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.
This is too long and shouldn't have a period.
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.
checkPhase = "${python.interpreter} runtests.py"; | ||
|
||
meta = with stdenv.lib; { | ||
homepage = "https://github.com/carltongibson/django-filter/tree/master"; |
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.
homepage = "https://github.com/carltongibson/django-filter/tree/master"; | |
homepage = "https://github.com/carltongibson/django-filter"; |
checkPhase = "${python.interpreter} test.py"; | ||
|
||
meta = with stdenv.lib; { | ||
homepage = "http://github.com/redis/hiredis-py"; |
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.
homepage = "http://github.com/redis/hiredis-py"; | |
homepage = "https://github.com/redis/hiredis-py"; |
channels | ||
]; | ||
|
||
checkInputs = [ pytest async_generator ]; |
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'm seeing more testing requirements here https://github.com/django/channels_redis/blob/2.4.0/setup.py#L12 (specifically cryptography).
Are then not needed in the code anymore?
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.
The path that uses cryptography
(https://github.com/django/channels_redis/blob/6f6ced8afa126b533fef32461b2ecfd1f8f5a0a3/channels_redis/core.py#L256) doesn't appear to be covered by the tests. There's probably a good argument for including cryptography
in propagatedBuildInputs
as it would enable that feature.
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.
yep it is an extra feature
I'd leave that up to the author to decide whether they want it enabled default or not.
doCheck = false; | ||
|
||
meta = with stdenv.lib; { | ||
homepage = "http://github.com/django/channels_redis"; |
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.
homepage = "http://github.com/django/channels_redis"; | |
homepage = "https://github.com/aio-libs/aioredis"; |
|
||
pname = "aioredis"; | ||
version = "1.2.0"; | ||
|
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.
disabled = isPy27; | |
{ stdenv | ||
, fetchPypi | ||
, buildPythonPackage | ||
, hiredis, async-timeout |
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.
, hiredis, async-timeout | |
, hiredis, async-timeout, isPy27 |
}; | ||
|
||
propagatedBuildInputs = [ | ||
hiredis |
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.
You need to add this conditioned on !isPyPy
https://github.com/aio-libs/aioredis/blob/ba9f5b707a29df5c90bc0a1c242d835564407894/setup.py#L9
Nothing happening so closing. |
Motivation for this change
Required packages for a project at work.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)