search

dendrograms / astrodendro

Generate a dendrogram from a dataset
https://dendrograms.readthedocs.io/
Other
37 stars 38 forks source link

Prevent use of dynamic ``__import__`` #3

Closed astrofrog closed 12 years ago

astrofrog commented 12 years ago

I would prefer if we didn't use __import__ because it feels a bit hacky, and is not relative-import friendly. What do you think about the following way? It does mean adding an entry to the main dendrogram.py file when a new file format is added, but I don't think it will happen very often.

This means moving the h5py and pyfits imports inside the functions to prevent ImportErrors if the dependencies are not installed, and also involves moving the imports of Dendrogram, Branch, and Leaf inside the import method to prevent circular imports.

cc @bradenmacdonald @ChrisBeaumont

ChrisBeaumont commented 12 years ago

Fine with me, though I have no strong opinion either way

bradenmacdonald commented 12 years ago

A lot of my python experience has been with Django, and that very large and respected software project uses __import__ and the like all the time, without any issues, so I felt like that was still relatively clean. If we were only supporting Python 2.7+ or even Python 3+ (not sure what min. Python version is?), I would think it best to use importlib.import_module, which lets you do dynamic relative imports.

However, I don't have a strong opinion on this either. Any way is fine with me.

astrofrog commented 12 years ago

@bradenmacdonald @ChrisBeaumont - the reason I think that __import__ is overkill here is because this is going to be a pretty stable package (We will probably only support FITS and HDF5), and users aren't going to be adding their I/O routines to astrodendro.io, so dynamic loading isn't really necessary. I'm not opposed to __import__, but I also don't see the advantage of that over the method in this PR, and I feel like it decreases the readability of the code. Let me turn the question around - are there any benefits to using __import__ here rather than the way I am suggesting?

ChrisBeaumont commented 12 years ago

I guess the advantage of @bradenmacdonald's approach is that he needs fewer import statements (since he delays importing modules with optional dependencies)

IMO, the biggest disadvantage to his approach is that it's harder to use tools like grep to find out which modules are getting used where. I don't see that as a big issue at the moment since the codebase is small.

astrofrog commented 12 years ago

@ChrisBeaumont - I see. Note that I'm also delaying the import of the dependencies (i.e. pyfits and h5py) until the I/O functions are actually called. I do import io.fits and io.hdf5, but not the dependencies themselves.

bradenmacdonald commented 12 years ago

@astrofrog Makes sense. I do agree with your comment that "import is overkill here is because this is going to be a pretty stable package...". Your pull request proposal thus is fine with me, although I would prefer the more concise syntax that I suggested in my inline comment above.

astrofrog commented 12 years ago

@bradenmacdonald - I've updated this PR, implementing your more concise notation (which I agree is better). Let me know if this is good to merge!

bradenmacdonald commented 12 years ago

:+1: Looks great! Cheers.