pearu / pylibnidaqmx

a Python wrapper to libnidaqmx library
Other
10 stars 9 forks source link

Split libnidaqmx.py into submodules #62

Open hoechenberger opened 8 years ago

hoechenberger commented 8 years ago

The libnidaqmx module is extremely large and hard to navigate; I suggest splitting it up into several smaller submodules.

What do you think @ert, @jpkotta, @pearu?

pearu commented 8 years ago

Hi,

Currently, libnidaqmx.py has about 4500 lines of code (large part of it is taken by documentation strings) and support for many NIDAQmx functions have not been implemented. So, I agree that the module should be factored into submodules.

Here's a list of required tasks:

  1. All code needed for "_header_name, libnidaqmx = _find_library()" should be moved under submodule, say, loader.py.
  2. Functions CHK, CALL, make_pattern could be moved under submodule utils.py
  3. Classes Device and System could be moved under device.py and system.py, respectively.
  4. The biggest class Task, and it can only grow when the support for missing functions will be implemented, could live under task.py or tasks.py. Task subclasses AnalogInputTask, etc. should probably be in the same module as the base class because these are related via DoneEventCallback_map, etc hooks.
  5. nidaqmxh*.py files could live under subdirectory, say, headers/.
  6. Is anybody using optparse_options.py or wxagg_plot.py? These could be moved under examples/ directory.
  7. Example scripts ai.py etc depend on ioc.optparse_gui which isn't available. The module optparse_gui.py should be copied from https://github.com/pearu/iocbio/blob/master/iocbio/optparse_gui.py to examples and make the example scripts working.
  8. version.py assumes svn, make it work with git
  9. Increase MINOR number in setup.py when receiving code refactoring patches.
  10. Create THANKS.txt file listing all contributors.

Best regards, Pearu

On Thu, May 12, 2016 at 8:35 PM, Richard Höchenberger < notifications@github.com> wrote:

The libnidaqmx is extremely large and hard to navigate; I suggest splitting it up into several smaller submodules.

What do you think @ert https://github.com/ert, @jpkotta https://github.com/jpkotta, @pearu https://github.com/pearu?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/pearu/pylibnidaqmx/issues/62

hoechenberger commented 8 years ago

Thanks @pearu!

I agree with all but one of your points -- even the suggested names for submodules are exactly like the ones I had in mind :) But re point 4,

The biggest class Task, and it can only grow when the support for missing functions will be implemented, could live under task.py or tasks.py. Task subclasses AnalogInputTask, etc. should probably be in the same module as the base class because these are related via DoneEventCallback_map, etc hooks.

I don't now if it wouldn't be better to split those up too, into tasks.py, analog.py, digital.py, counter.py. Or maybe: tasks_base.py, tasks_analog.py, tasks_digital.py, tasks_counter.py (I prefer the first naming scheme to the second) But I would have to dive some more into the code to see if this would really be a feasible approach :)

pearu commented 8 years ago

I like your approach as well. tasks.py, analog.py, etc are feasible when the modules are imported at the right order:

  1. analog.py imports tasks as usual (in the file header part)
  2. tasks.py imports analog.py, etc at the end of the file where the mappings are defined.

When tasks.py will only contain the definition of class Task, perhaps it ought to be named as task.py?

hoechenberger commented 8 years ago

Sounds good. Will try to look into this tomorrow :)

hoechenberger commented 8 years ago

I have started working on this; in fact, the only open tasks are

  1. version.py assumes svn, make it work with git
  2. Increase MINOR number in setup.py when receiving code refactoring patches.
  3. Create THANKS.txt file listing all contributors.

and testing.

To avoid circular imports, I'm importing the callback maps on method level in the Task class now. Not very pretty, but it's working.

I don't know anything about numpy.distutils, so cannot really work on 1. at this time.

I've created a PR for my current changes; please feel free to comment, test, fix problems you encounter: #63

pearu commented 8 years ago

I cannot fix problems right now but here are few comments/todos:

  1. nidaqmx/setup.py should have the following line inside configuration option:
config.add_subpackage('headers')

and the line

scripts = glob(join(config.local_path, 'scripts', '*.py'))

should now read

scripts = glob(join(config.local_path, 'examples', '*.py'))

I am also considering dropping script support (that is, updating sys.path) inside the setup.py file.

  1. I think we can drop svn support (that is, adding svn revision number to version string) as git does not have good equivalent to the svn revision number. We just need to update the MICRO version number more frequent.
  2. Feel free to increase MINOR.
  3. After refactoring, do your nidaqmx dependent applications work w/o updating them? I think that the requirement "users should not change their scripts after upgrading nidaqmx" is a prerequisite for merging the patches to the master branch.
hoechenberger commented 8 years ago

Thanks, I'll look into that.

Our lab is currently busy, so cannot test whether my scripts would run un-changed; I agree that we should try to stay backward-compatible though. Will do some "real world" testing asap.

FWIW, I've added a THANKS.txt file, but I'm under the impression some authors are missing.

PS D:\Development\pylibnidaqmx> git shortlog -s -n
    46  pearu.peterson
    22  Richard Höchenberger
     8  jpkotta
     5  richard.hoechenberger
     3  Jonathan Kotta
     3  dfgdfg
     2  Hendrik v. Raven

I have no idea who dfgdfg would be...