-
Notifications
You must be signed in to change notification settings - Fork 30
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
Selected yaml.Loader while reading odml documents #344
Conversation
Ach... Sorry for the delay... I am not sure if the coding style is fine or not. I'll wait for the tests and resolve them if there is any problem |
Hi. Thanks for the PR. This is great. I haven't looked at everything in detail yet, but the test failures are all for Python 2.7. Now, given the EOL status of Python2, we are planning on dropping support sometime soon. However, since we haven't yet decided exactly which release will be the last Python2-compatible one, I think we would want this fix to be compatible with both 3 and 2. Can you have a look at the issue and add it to the fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Small formatting issue with indentation.
Also, we should probably add the same change with the Loader
to yaml.load()
on L181 in the from_string()
method.
f69de9f
to
fd641be
Compare
Python 2 version of PyYaml seems to lack a constructor to load Unicode values by default using the SafeLoader (I don't know why... since the library is the same for Python 3 as well). I have added in a constructor to alleviate the problem, however, this problem exists only for Python 2 and hence the solution applies to Python 2 only. The constructor---in my assumption, should have no effect on Python 3 installations. We can also revert from using the SafeLoader to Loader itself, but I do not think it is worth the risk, and the given warning from the PyYaml team. |
Hey. Thanks again. I think |
You're right. I completely missed that loader. Fixing it. Anyhow, I was also looking at [EDIT]: Rebased and force pushed to squash small commits to one big commit. |
Added a constructor for Python 2 to load unicode objects and fixed PEP8 styling.
fd641be
to
48e55a2
Compare
With PR G-Node#344 the pyyaml dependency no longer needs to be fixed to version 4.2b4.
Fixes #343 .
Selecting a PyYaml loader instead of the default loader. The default loader fails on Python 3.8