search

labscript-suite / labscript-utils

Shared modules used by the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦. Includes a graphical exception handler, debug tools, configuration management, cross platform filepath conversions, unit conversions and custom GUI widgets.
http://labscriptsuite.org
Other
2 stars 45 forks source link

Fixed bug in unitconversions._All._import_all() #74

Closed zakv closed 2 years ago

zakv commented 3 years ago

Previously in unitconversions._All._import_all() , self.__all__ wasn't always changed from None to an empty list before appending things to it. In particular that wouldn't happen when explicitly calling ._All._import_all() instead of using its __getitem__() method first. That would raise AttributeError: 'NoneType' object has no attribute 'append'. This PR edits ._All._import_all() to ensure that it is always set to an empty list before appending things to it.

During startup, the blacs code here would suppress that error. It assumes that the AttributeError was raised because the desired unit conversion class didn't exist in the module, which would cause a somewhat misleading error message to be logged. Took me a while with a debugger to figure out that python can import the module, it just fails to import because _All._import_all() errors out first.

dihm commented 2 years ago

@zakv, assuming this all still looks good to you, I'm happy to merge this.

zakv commented 2 years ago

Maybe we should get @chrisjbillington's eyes on this first? From https://github.com/labscript-suite/labscript/issues/71 it seems like he might have un-merged changes related to importing unit conversion classes. Maybe he's already fixed it and merging this PR might create a merge conflict later on?

chrisjbillington commented 2 years ago

This fix is orthogonal to labscript-suite/labscript#71 (which I've just submitted a PR for - labscript-suite/labscript#84), so go ahead!