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

Allow loader to accept non-strings #80

Closed
wants to merge 4 commits into from

Conversation

clbarnes
Copy link
Contributor

@clbarnes clbarnes commented Jan 9, 2019

etree is old and so doesn't accept py3.4's pathlib.Path.
parsexml_ now calls str() on non-strings to work around this and other
similar os.PathLikes.

@clbarnes
Copy link
Contributor Author

clbarnes commented Jan 9, 2019

The 3.5 failure should be fixed by #81.

The 2.6 failure is because 2.6 has been EOL since I was in undergrad. See #79.

etree is old and so doesn't accept py3.4's `pathlib.Path`.
parsexml_ now calls `str()` on non-strings to work around this and other
similar `os.PathLike`s.
@clbarnes
Copy link
Contributor Author

clbarnes commented Jan 9, 2019

This has been rebased on #81 to fix the 3.5 failure.

@clbarnes
Copy link
Contributor Author

Hmm. Is this PR even mergeable, as nml.py is generated code? The changes will just break next time it's generated; so it's really a problem with generateDS.py.

@pgleeson
Copy link
Member

Thanks for these changes @clbarnes, i'll look at them over the next few days.

An update to the version of generateDS might solve some of the problems.. Alternatively a small script for a standard search/replace to post process the nml.py should suffice, with instructions added here: https://github.com/NeuralEnsemble/libNeuroML/blob/development/neuroml/nml/README.md.

@clbarnes
Copy link
Contributor Author

Thanks, I'll look into that. I think my commits are all in the wrong order now I've realised editing nml.py is the wrong way to go, so I'll close these PRs and cherry-pick the other changes.

@clbarnes clbarnes closed this Jan 10, 2019
@clbarnes
Copy link
Contributor Author

Just FYI I have raised a PR on generateds which should allow this the next time the code is regenerated.

https://bitbucket.org/dkuhlman/generateds/pull-requests/45/make-parsexml_-compatible-with-ospathlike/diff

@clbarnes
Copy link
Contributor Author

The generateds PR has been merged https://bitbucket.org/dkuhlman/generateds/commits/8ee5214bd106 , don't know when it'll make it into a release though.

@pgleeson
Copy link
Member

Thanks @clbarnes. Have tested the latest generateDS locally with libNeuroML and it looks like it works fine. Will check through your other changes as soon as I get a chance.

@clbarnes
Copy link
Contributor Author

Worth noting that the changes to generateDS will only allow it to work with Paths in python 3.6 up. It will still work with strings in anything lower than that.

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