txoof / PaperPi

E-Paper display loop with plugins
104 stars 10 forks source link

wrap plugin construction in try/except: #82

Closed txoof closed 7 months ago

txoof commented 1 year ago

I'm surprised this hasn't crashed out already.

def build_plugins_list(config, resolution, cache):
           ...
           # wrap this in a try statement
           my_plugin = Plugin(**plugin_config)
           # except...
            try:
                my_plugin.update()
            except AttributeError as e:
                logger.warning(f'ignoring plugin {my_plugin.name} due to missing update_function')
                logger.warning(f'plugin threw error: {e}')
                continue    
            logger.info(f'appending plugin {my_plugin.name}')

            plugins.append(my_plugin)

    return plugins
txoof commented 1 year ago

in file > PaperPi/paperpip/paperpi.py

Harshal662 commented 1 year ago

def build_plugins_list(config, resolution, cache):

for plugin_config in config:
    try:
        my_plugin = Plugin(**plugin_config)
    except Exception as e:
        logger.warning(f'ignoring plugin due to error: {e}')
        continue

    try:
        my_plugin.update()
    except AttributeError as e:
        logger.warning(f'ignoring plugin {my_plugin.name} due to missing update_function')
        logger.warning(f'plugin threw error: {e}')
        continue

    logger.info(f'appending plugin {my_plugin.name}')
    plugins.append(my_plugin)

return plugins

IS this good?

txoof commented 1 year ago

I think this is a bigger problem than I thought. What you propose will definitely work, but there's a bigger mess hiding in the Plugin class. Would you be interested in lookin into this? It's definitely more than a one-liner fix.

When init'd, the plugin can (or should) throw all sorts of exceptions. I think it really needs some better error handling with the setters. It might make sense to build an error class called PluginError as a catch-all for some common exceptions.

For example:

     def __init__(self, 
                 resolution, # should be a tuple of positive int, or throw TypeError or ValueError
                 name=None, 
                 layout={}, # should check if it's a dict, or throw TypeError 
                 update_function=None,  # should check if this is actually a function, or throw TypeError
                 max_priority = 2**15,  # should be a positive integer, or throw ValueError
                 refresh_rate=60, # should be positive integer, or throw Value/TypeError
                 min_display_time=30, # should be positive integer
                 config={}, # should be dict, or throw TypeError
                 cache=None, # should be None or CacheFiles object, or throw ValueError
                 force_onebit=False, # should be bool, or throw TypeError
                 screen_mode='1', # should be in epdlib.constants.modes or throw ValueError,
                 **kwargs):
Harshal662 commented 1 year ago

class PluginError(Exception): pass

class Plugin:

Plugin class implementation here...

def build_plugins_list(config, resolution, cache): plugins = []

for plugin_config in config:
    try:
        my_plugin = Plugin(resolution=resolution, cache=cache, **plugin_config)
    except PluginError as e:
        logger.warning(f'Ignoring plugin due to error: {e}')
        continue

    # Rest of the build_plugins_list function...

return plugins

Something like this?
txoof commented 1 year ago

Yeah, that would do the trick, but the changes need to be added into the Plugin library and then made so all the setters called in the class init ultimately throw a PluginError.

Check Plugin class here

Any chance you also work in Jupyter? I do most of my development in Jupyter to make it easier to test graphical output; it's easiest in the project workflow to update the .ipynb files and then produce the .py files from those.

Harshal662 commented 1 year ago

ok i will try to do that

txoof commented 1 year ago

paperpi.py: this also needs some wrapping in the event that bad data is passed to it from the initial config

def setup_splash(config, resolution):
    if config['main'].get('splash', False):
        logger.debug('displaying splash screen')
        from plugins.splash_screen import splash_screen
        splash_config = {
            'name': 'Splash Screen',
            'layout': splash_screen.layout.layout,
            'update_function': splash_screen.update_function,
            'resolution': resolution,
        }
        splash = Plugin(**splash_config)
        splash.update(constants.APP_NAME, constants.VERSION, constants.URL)
    else:
        logger.debug('skipping splash screen')
        splash = False

    return splash
Harshal662 commented 1 year ago

def setup_splash(config, resolution): try: splash_enabled = config['main'].get('splash', False) except (KeyError, TypeError): logger.error('Invalid config data: main.splash') return None

if not isinstance(splash_enabled, bool):
    logger.error('Invalid config data: main.splash must be a boolean')
    return None

if splash_enabled:
    logger.debug('displaying splash screen')
    try:
        from plugins.splash_screen import splash_screen
        splash_config = {
            'name': 'Splash Screen',
            'layout': splash_screen.layout.layout,
            'update_function': splash_screen.update_function,
            'resolution': resolution,
        }
        splash = Plugin(**splash_config)
        splash.update(constants.APP_NAME, constants.VERSION, constants.URL)
    except ImportError:
        logger.error('Failed to import splash_screen module')
        return None
    except Exception as e:
        logger.error('Error during splash screen setup: %s', str(e))
        return None
else:
    logger.debug('skipping splash screen')
    splash = None

return splash
txoof commented 7 months ago

152 closes this