Skip to content

Conversation

@achilleas-k
Copy link
Contributor

This PR adds a version compatibility check function to the NIX IO.

Currently the IO requires NIXPy (nixio) version 1.5 or greater (betas included). The version checker only checks the first two components of the version string.

The same function is used in the constructor of the NIXRawIO.

Some codestyle cleanup for the NIXRawIO is included.

@pep8speaks
Copy link

pep8speaks commented Jan 19, 2019

Hello @achilleas-k! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-26 18:48:18 UTC

nixverstr = nix.__version__
nixver = nixverstr.split(".")

try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it makes sense to use packaging.version for the version handling here instead of doing explicit string handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it and since the packaging module isn't in the standard library, I didn't want to add an extra dependency. I only now realise that it's part of the setuptools and pip group of modules so it's pretty close to standard.

I'll amend.

@achilleas-k
Copy link
Contributor Author

Regarding the errors, I noticed them when preparing this PR and opened an issue on our fork: G-Node#21

At the time it looked like it was only occurring on macOS but it looks like it's happening on circle as well. Haven't had the chance to investigate yet.

@achilleas-k
Copy link
Contributor Author

Amended to use packaging.version for comparisons.

To allow betas and dev releases of 1.5, the version comparison is made against the base_version of the installed nixio module.

If the version string is not understood for any reason, a warning is presented, but usage is allowed.

@achilleas-k
Copy link
Contributor Author

Looks like the travis environments don't include the packaging module.

@JuliaSprenger
Copy link
Member

Except for Py3.4? That's strange.

@achilleas-k
Copy link
Contributor Author

Quite strange. I also assumed circle was failing for the old reason, but it looks like those envs don't have the module either.

Currently the IO requires NIXPy (nixio) version 1.5.0 or greater (betas
included).  We use distutils.version to make the version string
comparison.  The NIXIO version string prefix 'v' is trimmed.
- Wrapped long lines
- Broke several long lines into multiple statements (deep nested
dictionary access)
- Fixed baserawio import (relative import path)
Run the same version check from the NixIO in the constructor of the
NIXRawIO
@achilleas-k achilleas-k force-pushed the nixio-version-checks branch from cf8b084 to 4f4e8a8 Compare July 26, 2019 18:48
@achilleas-k
Copy link
Contributor Author

Changed to distutils.version.

@apdavison apdavison added this to the 0.9.0 milestone Sep 24, 2019
@samuelgarcia samuelgarcia modified the milestones: 0.9.0, 0.8.0 Sep 24, 2019
@JuliaSprenger JuliaSprenger merged commit f17771a into NeuralEnsemble:master Sep 25, 2019
@achilleas-k achilleas-k deleted the nixio-version-checks branch December 11, 2019 13:53
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.

5 participants