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

Selected yaml.Loader while reading odml documents #344

Merged
merged 3 commits into from
Jan 17, 2020

Conversation

Aniket-Pradhan
Copy link
Contributor

Fixes #343 .

Selecting a PyYaml loader instead of the default loader. The default loader fails on Python 3.8

@coveralls
Copy link

coveralls commented Jan 4, 2020

Coverage Status

Coverage increased (+0.02%) to 75.129% when pulling 48e55a2 on Aniket-Pradhan:master into 3ebada5 on G-Node:master.

@Aniket-Pradhan
Copy link
Contributor Author

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

@achilleas-k
Copy link
Member

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?

Copy link
Member

@achilleas-k achilleas-k left a 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.

odml/tools/odmlparser.py Outdated Show resolved Hide resolved
@Aniket-Pradhan
Copy link
Contributor Author

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.

@achilleas-k
Copy link
Member

Hey. Thanks again.

I think yaml.load() in the from_string() method should also use the SafeLoader for the same reasons.

@Aniket-Pradhan
Copy link
Contributor Author

Aniket-Pradhan commented Jan 16, 2020

You're right.

I completely missed that loader. Fixing it.

Anyhow, I was also looking at yaml.safe_dump, but it cannot represent arbitrary python objects (datetime.date), therefore we cannot use it unless the devs fix the datetime.date and datetime.time inconsistency.

[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.
@mpsonntag mpsonntag merged commit b748339 into G-Node:master Jan 17, 2020
mpsonntag added a commit to mpsonntag/python-odml that referenced this pull request Jan 22, 2020
With PR G-Node#344 the pyyaml dependency no longer needs
to be fixed to version 4.2b4.
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.

Test failiure on Python 3.8
4 participants