close
Skip to content

Update 'add_ons_to_gss' command to support CSVReader for Python 2#244

Closed
davidbasalla wants to merge 1 commit into
mysociety:masterfrom
davidbasalla:update-add-ons-to-gss-script-to-support-python-2
Closed

Update 'add_ons_to_gss' command to support CSVReader for Python 2#244
davidbasalla wants to merge 1 commit into
mysociety:masterfrom
davidbasalla:update-add-ons-to-gss-script-to-support-python-2

Conversation

@davidbasalla
Copy link
Copy Markdown
Contributor

The command would error when running with Python 2.7, failing to recognise the Welsh characters ô and â with "UnicodeEncodeError: 'ascii' codec can't encode character u'\xf4' in position 16: ordinal not in range(128)". This is due to csv.reader changing between Python 2 and 3 - in 2 it expected a file-like object on which next() would return raw bytes, and in 3 one which would return unicode strings (found by @jennyd). We opt to explicitly check which version of Python is running and run the old code for < Python 3.

We also stop assigning name for the 'code-change.csv' as it is not used anywhere else in the script and the encoding probably won't be handled correctly with Python 2. We stop assigning type and name for 'missing-codes.csv' for the same reasons.

The command would error when running with Python 2.7, failing to recognise the Welsh characters `ô` and `â` with "UnicodeEncodeError: 'ascii' codec can't encode character u'\xf4' in position 16: ordinal not in range(128)". This is due to `csv.reader` changing between Python 2 and 3 - in 2 it expected a file-like object on which next() would return raw bytes, and in 3 one which would return unicode strings. We opt to explicitly check which version of Python is running and run the old code for < Python 3.

We also stop assigning `name` for the 'code-change.csv' as it is not used anywhere else in the script and the encoding probably won't be handled correctly with Python 2. We stop assigning `type` and `name` for 'missing-codes.csv' for the same reasons.
@dracos dracos added the Current label Jun 28, 2016
@dracos
Copy link
Copy Markdown
Member

dracos commented Jun 29, 2016

Hi, thanks very much for this :) It's not a script we run any more upstream, so hadn't spotted this.

I have pushed a refactor fixup commit at 2e341a0 which I think simplifies this somewhat – could you check that it is still okay for you? If so, please do squash the two commits together in your branch. Could you also wrap your commit message to the standard 50/72 line lengths; thank you!

@dracos dracos added Reviewed and removed Current labels Jun 29, 2016
@dracos
Copy link
Copy Markdown
Member

dracos commented Sep 30, 2016

This has been merged as d659d40 (with my change), thanks!

@dracos dracos closed this Sep 30, 2016
@dracos dracos removed the Reviewed label Sep 30, 2016
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.

2 participants