search

crs4 / hl7apy

Python library to parse, create and handle HL7 v2 messages.
http://crs4.github.io/hl7apy/
MIT License
231 stars 91 forks source link

Updated to work with Python 3. #19

Closed eheinz closed 7 years ago

eheinz commented 9 years ago

This pull request adds support for python 3. I chose to use the try/except approach rather than adding any external dependencies like six or python-future. I also leveraged the added support for unicode literals in python 3.3 (which substantially reduced the amount of code that needed to be altered). As a result, it works on python 3.3+.

landscape-bot commented 9 years ago

Code Health Repository health decreased by 0.05% when pulling d220845 on setaris:python3 into 934c5d4 on crs4:develop.

svituz commented 9 years ago

Hi eheinz, thanks for your work. I like your approach, since it doesn't add requirements to the library which in our case can be avoided. I think there is still some work to do to achieve the goal. I launched the tests using python3.4 and a lot of them failed, mostly because of the problem with the function load_message_profile which fails (I wrote a comment for that). There are also problem issues that I list here:

  1. the print statements need to be converted to the python3 function
  2. the mllp module also fails in the tests because of the same problem as the load_message_profile function: the socket functions send bytes so the str objects need to be encoded/decoded in python3 (have a look at the 4th point here
  3. the base_datatypes module has a problem at line 149: the cmp key for sorted function is not valid anymore in Python 3. It should be substituted with key=functools.cmp_to_key(_sort_highlights)
  4. probably it is better to use the from __future__ import unicode_literals to use unicode also in python 2 (I have to investigate deeply this last, I'm new to Python 3 and versions compatibility).
  5. the utils script should be updated to support Python 3 (at least hl7apy_profile_parser).
  6. the travis.yml file should be updated to include tests also for Python3

Since this is a deep change for the library, I'd rather merge your changes in a new branch that I will open. Maybe you can update your pull request to fix the first 3 points and, when the tests are ok both for python 2 and 3, I will merge your changes in the new branch.

Thanks again, Vittorio

JonathonReinhart commented 7 years ago

I think this MR can be closed now since #23 was merged.

svituz commented 7 years ago

As @JonathonReinhart suggested, I'm closing it after #23