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

pythonPackages.{hiredis,aioredis,channels_redis,django-crispy-forms,django-filter}: init; Added baddecisionsalex to maintainers. #63545

Closed
wants to merge 6 commits into from

Conversation

aakropotkin
Copy link
Contributor

Motivation for this change

Required packages for a project at work.

Things done
  • adding BadDecisionsAlex to maintainer's list.
  • pythonPackages.django-filter: init at 2.1.0
  • pythonPackages.django-crispy-forms: init at 1.7.2
  • pythonPackages.channels_redis: init at 2.4.0
  • pythonPackages.aioredis: init at 1.2.0
  • pythonPackages.hiredis: init at 1.0.0
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

We have an eval failure

while evaluating the attribute 'maintainers' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/pkgs/development/libraries/rocksdb/default.nix:41:5:
undefined variable 'magenbluten' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/pkgs/development/libraries/rocksdb/default.nix:41:44

Looks like a entry was removed from the maintainers list in 59446b9

@aakropotkin
Copy link
Contributor Author

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?
Im by no means a rebase veteran 😂

@risicle
Copy link
Contributor

risicle commented Jun 27, 2019

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. TMP aioredis) and then doing an interactive rebase (git rebase -i <base commit>) to reorder the commits and fixup the temporary ones, folding them into their corresponding original commits.

@aakropotkin
Copy link
Contributor Author

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.

@aakropotkin
Copy link
Contributor Author

@FRidh I think that might have wrapped this one up.
Let me know if you want any additional revisions.

@risicle
Copy link
Contributor

risicle commented Jun 27, 2019

I think you might need to take another stab at the "add to maintainers" commit, making it only add yourself.

@risicle
Copy link
Contributor

risicle commented Jun 27, 2019

(nox-review is happy for me now, non-nixos linux x86_64)

@aakropotkin
Copy link
Contributor Author

I think you might need to take another stab at the "add to maintainers" commit, making it only add yourself.

Hey I finally got that fixed :)

Copy link
Contributor

@worldofpeace worldofpeace left a 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.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.";
Copy link
Contributor

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.

Copy link
Contributor

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
homepage = "http://github.com/redis/hiredis-py";
homepage = "https://github.com/redis/hiredis-py";

channels
];

checkInputs = [ pytest async_generator ];
Copy link
Contributor

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?

Copy link
Contributor

@risicle risicle Jul 12, 2019

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.

Copy link
Contributor

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
homepage = "http://github.com/django/channels_redis";
homepage = "https://github.com/aio-libs/aioredis";


pname = "aioredis";
version = "1.2.0";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
disabled = isPy27;

{ stdenv
, fetchPypi
, buildPythonPackage
, hiredis, async-timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, hiredis, async-timeout
, hiredis, async-timeout, isPy27

};

propagatedBuildInputs = [
hiredis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FRidh
Copy link
Member

FRidh commented Aug 17, 2019

Nothing happening so closing.

@FRidh FRidh closed this Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants