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

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

Conversation

davidbasalla
Copy link
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
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
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