Skip to content

Conversation

@tacatac
Copy link
Contributor

@tacatac tacatac commented Apr 25, 2019

Rationale

On Archlinux python points to Python3 but I can't be bothered to sed -i -e 's/python/python2/g' Makefile generate_style.py.
Also I need generate_style.py to work on a system where python is Python2.

Modifications

Some are for deprecated or removed functions (optparse, iteritems, has_key).
Some are recommendations from PyLint (although I did not start on the spacing and indentation warnings!): vars renaming, isinstance, unused sys import.
print could be modified without needing __future__.

Testing

I've not done much except checksumming the resulting mapfiles to make sure I generated the same one after each change.
As far performance is concerned (observed with time -v make on my system) there is no significant difference. A full run takes about 2 seconds (90% usr), with max RSS 17.5MiB on Python2, with or without these changes.
It runs at 1 second, same RSS with Python3.

tacatac added 9 commits April 25, 2019 20:36
* use update() method rather than using a list copy intermediary
* the main dictionary could be copied first but it isn't used for
anything else
* supported by Python2 and 3
* not as efficient as an iterable but cursory `time -v` tests show no
difference in usr/system time or maximum RSS: presumably the style
dictionary would have to grow monumentally before we could see a
difference
* `vars` is a Python builtin function: dict renamed to 'style'
* 'styles' dictionary renamed to 'namedstyles' to avoid confusion
@jachym
Copy link
Contributor

jachym commented Nov 8, 2019

Note: I've updated generate_style.py to Python3 in the PR #68

@yjacolin
Copy link
Collaborator

I am ok to merge this PR (almost 1 year after, sorry) but I am not a Python dev.

@yjacolin yjacolin requested review from tbonfort and yjacolin March 10, 2020 09:19
@jmckenna
Copy link
Member

I think Python3 compatibility is important - is this PR ok to merge? (it's not clear to me how this affects #68 as both add v3 support)

@jmckenna jmckenna merged commit 0c21924 into MapServer:master Aug 5, 2020
@jmckenna
Copy link
Member

jmckenna commented Aug 5, 2020

thanks @tacatac !!

@jmckenna jmckenna mentioned this pull request Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants