python3 support #3816

samaiyou · 2019-06-29T07:45:54Z

Changes proposed in this pull request:

python3 support.
changed from iteritems to items on dict(), lxml.etree.tostring returns to bytes, so convert to utf-8.

StyXman

Looks good to me besides my comment. Maybe later we could give it another pass trying to make everything consistent (I still see some extra spaces that were in the original).

StyXman · 2019-07-01T19:25:59Z

scripts/generate_road_colours.py

@@ -32,7 +32,10 @@ def rgb_error(self):

 def load_settings():
    """Read the settings from YAML."""
-    return yaml.load(open('road-colors.yaml', 'r'))
+    with open('road-colors.yaml', 'r') as fd:
+        y=yaml.load(fd, Loader=yaml.SafeLoader)


Nitpick: could you add spaces around = like in the rest of the file?

autopep8 should catch such formatting, and fix most of them

If you want to enforce pep8 (any codestyle), check should be added to CI (.travis.yml)

But of course this should not be part of this PR.

matkoniecz · 2019-07-12T21:41:01Z

Sorry for a long delay!

It seems to me that scripts/generate_road_colours.py used to output elements in constant order and now it is doing in random order.

Though maybe earlier version worked by accident thanks to an undocumented behaviour?

meased · 2019-07-16T23:08:47Z

This behaviour is coming from the YAML loader, which has changed since Python 2:
yaml/pyyaml#110

This Python 3 version is listing out items in the order they are listed in the YAML file. The Python 2 version was essentially randomized (I believe it would hash the keys and then sort the hashes, but don't quote me on that). I would argue that this version is correct and the previous Python 2 version is "wrong".

Sjord · 2019-07-31T16:10:28Z

Beside the ordering, the colors are also different for me. I have no explanation for this.

python 2: @trunk-low-zoom-casing: #cf6649;
python 3: @trunk-low-zoom-casing: #d1684a;

Sjord · 2019-08-01T07:07:26Z

I have figured out why the colors are different:

delta_l = (line_colour_info.end_l - line_colour_info.start_l) / colour_divisions

In Python 2, this is an integer division: 20/3 = 6.
In Python 3, this is a float division: 20/3 = 6.667

pnorman

After changing the scripts the files they generate need to be regenerated

pnorman · 2019-08-01T18:41:54Z

Could you also change indexes.py to call python3

mboeringa · 2019-08-09T08:16:21Z

I have figured out why the colors are different:
delta_l = (line_colour_info.end_l - line_colour_info.start_l) / colour_divisions
In Python 2, this is an integer division: 20/3 = 6.
In Python 3, this is a float division: 20/3 = 6.667

@Sjord

Have you considered using the decimal module of Python?:
https://docs.python.org/2/library/decimal.html

pnorman · 2019-08-09T16:23:08Z

Have you considered using the decimal module of Python?:
https://docs.python.org/2/library/decimal.html

I don't see that decimal would give us any practical advantages over floats here.

mboeringa · 2019-08-09T16:35:17Z

I don't see that decimal would give us any practical advantages over floats here.

The calculations would be consistent between Python 2 and 3. But of course, if you are dropping Python 2 support entirely, which is logical given the end of life status, then yes, there isn't much to gain.

prmtl · 2019-08-09T18:09:08Z

@mboeringa It is not needed to use decimal module to have consistent division. Just use __future__ module and replace / with //:

# python 2 & 3
> from __future__ import division
> 20 // 3
6

pnorman · 2019-08-10T17:51:33Z

I've addressed the PR comments, but still need to get Travis working with python3

pnorman · 2019-08-10T18:45:02Z

This Python 3 version is listing out items in the order they are listed in the YAML file. The Python 2 version was essentially randomized (I believe it would hash the keys and then sort the hashes, but don't quote me on that). I would argue that this version is correct and the previous Python 2 version is "wrong".

Either would be correct, but the problem is that Python 3 prior to Python 3.7 doesn't have a fixed ordering. I think the options here are to require Python 3.7 or load into an OrderedDict.

Failing travis

lxml tostring returns bytes, so change bytes to utf8, iteritems had been gone, so cahange to imtes().

change iteritems to items

jeisenbe · 2019-09-17T01:41:25Z

@pnorman is this nearly ready to release?

pnorman · 2019-09-17T02:58:29Z

@pnorman is this nearly ready to release?

It needs reviewing.

matkoniecz · 2019-11-08T22:18:35Z

This PR changes 665 files due to removal of ".0" in generated SVGs. I am considering adding special additional code to preserve it for now, to avoid changing so many files and potentially drowning other changes.

pnorman · 2020-01-28T06:54:20Z

It needs reviewing.

To be clear, it needs reviewing by someone other than me since I took the PR over

Sjord

The script get-shapefiles.py wasn't changed in this PR, but it seems that it already supported Python 3. I have made some comments, but otherwise this PR looks good to me.

Sjord · 2020-01-31T16:19:18Z

scripts/generate_road_colours.py

@@ -32,7 +32,10 @@ def rgb_error(self):

 def load_settings():
    """Read the settings from YAML."""
-    return yaml.load(open('road-colors.yaml', 'r'))
+    with open('road-colors.yaml', 'r') as fd:
+        y = yaml.load(fd, Loader = yaml.SafeLoader)


Couldn't you return immediately here, without assigning to y in between?

Sjord · 2020-01-31T16:28:58Z

scripts/generate_shields.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3

 # Generate highway shields as SVG files in symbols/shields.



Line 5 says:

from __future__ import print_function

but this can be removed now. In other files as well.

Sjord · 2020-01-31T16:33:24Z

.travis.yml

 env:
  - CARTO=0.18.0 MAPNIK='3.0.0 3.0.12'
 install:
  - npm install carto@$CARTO
-  - pip install --user colormath
+  - pip3 install --user colormath


Shouldn't this also install PyYAML and lxml?

Sjord · 2020-01-31T16:37:17Z

.travis.yml

-    - python-yaml
-    - postgresql-9.4-postgis-2.3
+    - python3-yaml
+    - postgresql-9.4-postgis-2.4


Does this package exist? In bionic there seems to be postgresql-10-postgis-2.4.

pnorman · 2020-02-13T04:16:15Z

I'm going to close this. The original author never addressed the comments, and while I attempted to do some of them, there are still some left. I'd rather split the python 3 upgrade to multiple PRs, and have been doing so.

StyXman suggested changes Jul 1, 2019

View reviewed changes

pnorman requested changes Aug 1, 2019

View reviewed changes

pnorman mentioned this pull request Aug 1, 2019

Check for drift in generated MSS and SVGs #3830

Open

pnorman force-pushed the feat/python3 branch 3 times, most recently from 274f913 to d30a276 Compare August 10, 2019 17:50

pnorman force-pushed the feat/python3 branch from d30a276 to cb4894b Compare August 10, 2019 17:55

pnorman previously approved these changes Aug 10, 2019

View reviewed changes

pnorman self-requested a review August 10, 2019 17:56

pnorman force-pushed the feat/python3 branch 3 times, most recently from 2d8a5b5 to 1464d3f Compare August 10, 2019 18:12

samaiyou and others added 6 commits August 11, 2019 19:50

fix to python3

df50ec3

update support to python3

73f87e4

lxml tostring returns bytes, so change bytes to utf8, iteritems had been gone, so cahange to imtes().

support for python3

8de824e

change iteritems to items

Address PR comments

e536b93

Regenerate script-generated files

3287e1e

Move Travis to Python3

aaed1f9

pnorman force-pushed the feat/python3 branch 2 times, most recently from fbb8683 to 088a14e Compare August 12, 2019 03:02

Sort road colour output

4c80965

pnorman force-pushed the feat/python3 branch from 088a14e to 4c80965 Compare August 12, 2019 03:05

jeisenbe added code enhancement labels Sep 9, 2019

pnorman mentioned this pull request Jan 31, 2020

Switch Travis to Xenial #4018

Merged

Sjord reviewed Jan 31, 2020

View reviewed changes

pnorman closed this Feb 13, 2020

pnorman mentioned this pull request Mar 8, 2020

Upgrade generate_road_colours to Python3 #4062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python3 support #3816

python3 support #3816

samaiyou commented Jun 29, 2019

StyXman left a comment

StyXman Jul 1, 2019

matkoniecz Jul 1, 2019

prmtl Aug 4, 2019

prmtl Aug 4, 2019

matkoniecz commented Jul 12, 2019

meased commented Jul 16, 2019

Sjord commented Jul 31, 2019

Sjord commented Aug 1, 2019

pnorman left a comment

pnorman commented Aug 1, 2019

mboeringa commented Aug 9, 2019

pnorman commented Aug 9, 2019

mboeringa commented Aug 9, 2019

prmtl commented Aug 9, 2019

pnorman commented Aug 10, 2019

pnorman commented Aug 10, 2019

jeisenbe commented Sep 17, 2019

pnorman commented Sep 17, 2019

matkoniecz commented Nov 8, 2019

pnorman commented Jan 28, 2020

Sjord left a comment

Sjord Jan 31, 2020

Sjord Jan 31, 2020

Sjord Jan 31, 2020

Sjord Jan 31, 2020

pnorman commented Feb 13, 2020

python3 support #3816

python3 support #3816

Conversation

samaiyou commented Jun 29, 2019

Changes proposed in this pull request:

StyXman 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

matkoniecz commented Jul 12, 2019

meased commented Jul 16, 2019

Sjord commented Jul 31, 2019

Sjord commented Aug 1, 2019

pnorman left a comment

Choose a reason for hiding this comment

pnorman commented Aug 1, 2019

mboeringa commented Aug 9, 2019

pnorman commented Aug 9, 2019

mboeringa commented Aug 9, 2019

prmtl commented Aug 9, 2019

pnorman commented Aug 10, 2019

pnorman commented Aug 10, 2019

jeisenbe commented Sep 17, 2019

pnorman commented Sep 17, 2019

matkoniecz commented Nov 8, 2019

pnorman commented Jan 28, 2020

Sjord 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

pnorman commented Feb 13, 2020