python3Packages.geopy: 1.20 -> unstable-2019-11-10 #73158

GuillaumeDesforges · 2019-11-10T13:23:50Z

Motivation for this change

geopy is not compatible with Python 3, see issue #73155.

Things done

There is now a switch depending on Python version:

If Python is 2.7, keep like before.
If Python is 3.7, fetch package from Github as the 2.0 version is not yet release.

Notify maintainers

No maintainer found in package declaration.

JohnAZoidberg · 2019-11-10T13:56:51Z

pkgs/development/python-modules/geopy/default.nix

  };

-  doCheck = false;  # too much
+  geopy37 = {
+    version = "2.0.0";


If they haven't released 2.0.0 you cannot call it that. Please mark it as unstable and add the date as version. See the manual

Didn't know about the naming convention, sorry.

JohnAZoidberg · 2019-11-10T13:58:21Z

pkgs/development/python-modules/geopy/default.nix

-    description = "Python Geocoding Toolbox";
-    license = licenses.mit;
-    broken = true;
+    buildInputs = commonBuildInputs ++ [ geographiclib ];


Suggested change

buildInputs = commonBuildInputs ++ [ geographiclib ];

buildInputs = commonBuildInputs;

Anything in propagatedBuildInputs doesn't need to be added to buildInputs.

JohnAZoidberg · 2019-11-10T13:58:59Z

pkgs/development/python-modules/geopy/default.nix


-  buildInputs = [ mock tox pylint ];
+    src = fetchFromGitHub {
+      owner  = "geopy";


Suggested change

owner = "geopy";

owner = pname;

repo = pname;

FRidh

Please create two separate expressions, one called 2.nix for Python 2 and default.nix for Python 3.

GuillaumeDesforges · 2019-11-10T21:06:05Z

Please create two separate expressions, one called 2.nix for Python 2 and default.nix for Python 3.

What would be the best way to do that ? Do I setup most parameters in default.nix and then override in 2.nix, or do I put conditions in default.nix importing 2.nix ?

FRidh · 2019-11-11T09:39:34Z

Remove the common. Just consider them as if they were two separate packages. Then, in python-packages.nix you import and call them.

JohnAZoidberg

Please test the package again after making the changes and report what testing you have done. (The examples in the README seem reasonable)

JohnAZoidberg · 2019-11-11T18:00:36Z

pkgs/development/python-modules/geopy/default.nix

+    meta = with stdenv.lib; {
+        homepage = "https://github.com/geopy/geopy";
+        description = "Python Geocoding Toolbox";
+        license = licenses.mit;


Would you like to be the maintainer for this package?

If this is of any help, sure

Yes, it would be great if somebody updated/changed this package that you could be a second reviewer, who is familiar with this package.

JohnAZoidberg · 2019-11-11T18:01:52Z

pkgs/development/python-modules/geopy/default.nix

-  pname = "geopy";
-  version = "1.20.0";
-  disabled = !isPy27;
+    pname = "geopy";


Suggested change

pname = "geopy";

pname = "geopy-unstable";

Didn't understand the first time, thanks

JohnAZoidberg · 2019-11-11T18:01:53Z

pkgs/development/python-modules/geopy/default.nix

-  version = "1.20.0";
-  disabled = !isPy27;
+    pname = "geopy";
+    version = "2.0-unstable-2019-11-10";


Suggested change

version = "2.0-unstable-2019-11-10";

version = "2019-11-10";

JohnAZoidberg · 2019-11-11T18:04:54Z

pkgs/development/python-modules/geopy/default.nix

-    sha256 = "9419bc90ee6231590c4ae7acf1cf126cefbd0736942da7a6a1436946e80830e2";
-  };
+    disabled = !isPy3k; # only Python 3
+    doCheck = false; # too many checks


Suggested change

doCheck = false; # too many checks

doCheck = false; # Needs network access

I tried to run them because "too many checks" doesn't sound like a good excuse not to run them unless they take a considerable amount of time.

Fair enough, this is just what the comment from the original derivation sounded like to me, my bad

JohnAZoidberg · 2019-11-11T18:07:18Z

pkgs/development/python-modules/geopy/default.nix


-  doCheck = false;  # too much
+    buildInputs = [ mock tox pylint ];


Suggested change

buildInputs = [ mock tox pylint ];

They're only needed for the tests and since we don't run them ...
Please also remove in the derivation arguments and the other version.

Can't I just add them in checkInputs in case doCheck gets changed in the future ?

Hmm, you could. But it would likely be out of date then.
It looks out of date even now: https://github.com/geopy/geopy/blob/master/setup.py

So I'd just remove it.
Also do you expect them to remove the dependency on network access? Looks like this package needs network access for everything 😆

GuillaumeDesforges · 2019-11-11T22:28:20Z

Please test the package again after making the changes and report what testing you have done. (The examples in the README seem reasonable)

Before every commit I have checked runing the following:

[nix-shell:~/tmp/nixpkgs]$ python
Python 3.7.5 (default, Oct 14 2019, 23:08:55) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from geopy.geocoders import Nominatim
>>> geolocator = Nominatim(user_agent="specify_your_app_name_here")
>>> location = geolocator.geocode("Paris")
>>> print((location.latitude, location.longitude))
(48.8566101, 2.3514992)

JohnAZoidberg · 2019-11-12T16:38:38Z

pkgs/development/python-modules/geopy/2.nix

+        homepage = "https://github.com/geopy/geopy";
+        description = "Python Geocoding Toolbox";
+        license = licenses.mit;
+        maintainers = [ "aceus02@gmail.com" ];


Suggested change

maintainers = [ "aceus02@gmail.com" ];

maintainers = with maintainers; [ GuillaumeDesforges ];

Thanks for volunteering :)
Please create an entry for yourself in https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix

JohnAZoidberg · 2019-11-12T16:42:50Z

pkgs/development/python-modules/geopy/default.nix


-  doCheck = false;  # too much
+    buildInputs = [ mock tox pylint ];


Hmm, you could. But it would likely be out of date then.
It looks out of date even now: https://github.com/geopy/geopy/blob/master/setup.py

So I'd just remove it.
Also do you expect them to remove the dependency on network access? Looks like this package needs network access for everything 😆

JohnAZoidberg

Looks almost good :)
Please address the comments and change the indentation back to two spaces instead of 4. See the manual.

jonringer

do you mind squashing the commits? I would do two, one where you freeze the python2 version, and another where you bump the python3 version, otherwise looks okay...

I would prefer to have some unit tests, generally these don't require network access.

GuillaumeDesforges · 2019-11-12T18:17:38Z

Looks almost good :)
Please address the comments and change the indentation back to two spaces instead of 4. See the manual.

Thanks, this has been done.

do you mind squashing the commits? I would do two, one where you freeze the python2 version, and another where you bump the python3 version, otherwise looks okay...

I would prefer to have some unit tests, generally these don't require network access.

Squashed into two as asked (nice git exercise)

It seems that it would be hard to override pytest to run only the tests that do not require networking.

* Update geopy to unstable version from GitHub for Python 3+ * Add GuillaumeDesforges as maintainer

JohnAZoidberg

Thanks a lot Guillaume!
I took the liberty to adjust the commit title slightly (add the pythonXPackages.) and remove two trailing spaces that I didn't spot in GitHub.

I tested it again in Python 2 and 3. Both work fine with the example that you gave. :)

GuillaumeDesforges · 2019-11-15T07:46:23Z

@JohnAZoidberg thank you very much for your insightful reviews, hope I can be of help in the future

GuillaumeDesforges requested review from FRidh and jonringer as code owners November 10, 2019 13:23

GuillaumeDesforges changed the title ~~geopy: 1.2.0 -> 2.80 for Python 3 only~~ geopy: 1.2.0 -> 2.0 for Python 3 only Nov 10, 2019

ofborg bot added 6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Nov 10, 2019

JohnAZoidberg requested changes Nov 10, 2019

View reviewed changes

FRidh requested changes Nov 10, 2019

View reviewed changes

GuillaumeDesforges requested a review from JohnAZoidberg November 10, 2019 21:40

GuillaumeDesforges requested a review from FRidh November 11, 2019 12:23

JohnAZoidberg requested changes Nov 11, 2019

View reviewed changes

GuillaumeDesforges requested a review from JohnAZoidberg November 11, 2019 22:42

JohnAZoidberg reviewed Nov 12, 2019

View reviewed changes

JohnAZoidberg requested changes Nov 12, 2019

View reviewed changes

jonringer reviewed Nov 12, 2019

View reviewed changes

ofborg bot added the 11.by: package-maintainer label Nov 12, 2019

GuillaumeDesforges requested a review from JohnAZoidberg November 12, 2019 19:10

Guillaume Desforges added 2 commits November 14, 2019 23:18

python2Packages.geopy: Freeze in seperate file

d11137e

python3Packages.geopy: 1.20 -> unstable-2019-11-10

6d12f06

* Update geopy to unstable version from GitHub for Python 3+ * Add GuillaumeDesforges as maintainer

JohnAZoidberg approved these changes Nov 14, 2019

View reviewed changes

JohnAZoidberg changed the title ~~geopy: 1.2.0 -> 2.0 for Python 3 only~~ python3Packages.geopy: 1.20 -> unstable-2019-11-10 Nov 14, 2019

JohnAZoidberg merged commit c4ed79e into NixOS:master Nov 14, 2019

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python3Packages.geopy: 1.20 -> unstable-2019-11-10 #73158

python3Packages.geopy: 1.20 -> unstable-2019-11-10 #73158

GuillaumeDesforges commented Nov 10, 2019 •

edited

JohnAZoidberg Nov 10, 2019

GuillaumeDesforges Nov 10, 2019

JohnAZoidberg Nov 10, 2019

GuillaumeDesforges Nov 10, 2019

JohnAZoidberg Nov 10, 2019

GuillaumeDesforges Nov 10, 2019

FRidh left a comment

GuillaumeDesforges commented Nov 10, 2019

FRidh commented Nov 11, 2019

JohnAZoidberg left a comment

JohnAZoidberg Nov 11, 2019

GuillaumeDesforges Nov 11, 2019

JohnAZoidberg Nov 12, 2019

JohnAZoidberg Nov 11, 2019

GuillaumeDesforges Nov 11, 2019

JohnAZoidberg Nov 11, 2019

JohnAZoidberg Nov 11, 2019

GuillaumeDesforges Nov 11, 2019

JohnAZoidberg Nov 11, 2019

GuillaumeDesforges Nov 11, 2019

JohnAZoidberg Nov 12, 2019

GuillaumeDesforges commented Nov 11, 2019

JohnAZoidberg Nov 12, 2019

JohnAZoidberg Nov 12, 2019

JohnAZoidberg left a comment

jonringer left a comment

GuillaumeDesforges commented Nov 12, 2019

JohnAZoidberg left a comment

GuillaumeDesforges commented Nov 15, 2019

	buildInputs = commonBuildInputs ++ [ geographiclib ];
	buildInputs = commonBuildInputs;

	doCheck = false; # too many checks
	doCheck = false; # Needs network access


		doCheck = false; # too much
		buildInputs = [ mock tox pylint ];

	maintainers = [ "aceus02@gmail.com" ];
	maintainers = with maintainers; [ GuillaumeDesforges ];

python3Packages.geopy: 1.20 -> unstable-2019-11-10 #73158

python3Packages.geopy: 1.20 -> unstable-2019-11-10 #73158

Conversation

GuillaumeDesforges commented Nov 10, 2019 • edited

Motivation for this change

Things done

Notify maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Nov 10, 2019

FRidh commented Nov 11, 2019

JohnAZoidberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnAZoidberg left a comment

Choose a reason for hiding this comment

jonringer left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Nov 12, 2019

JohnAZoidberg left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Nov 15, 2019

GuillaumeDesforges commented Nov 10, 2019 •

edited