Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrap plugin construction in try/except: #82

Closed
txoof opened this issue Mar 1, 2023 · 9 comments
Closed

wrap plugin construction in try/except: #82

txoof opened this issue Mar 1, 2023 · 9 comments
Labels
good first issue Good for newcomers Work To Do Work that needs to be done

Comments

@txoof
Copy link
Owner

txoof commented Mar 1, 2023

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
Copy link
Owner Author

txoof commented Mar 1, 2023

in file > PaperPi/paperpip/paperpi.py

@txoof txoof added good first issue Good for newcomers Work To Do Work that needs to be done labels Jun 19, 2023
@Harshal662
Copy link
Contributor

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
Copy link
Owner Author

txoof commented Jun 22, 2023

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
Copy link
Contributor

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
Copy link
Owner Author

txoof commented Jun 22, 2023

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
Copy link
Contributor

ok i will try to do that

@txoof
Copy link
Owner Author

txoof commented Jun 23, 2023

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
Copy link
Contributor

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
Copy link
Owner Author

txoof commented Mar 12, 2024

#152 closes this

@txoof txoof closed this as completed Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Work To Do Work that needs to be done
Projects
None yet
Development

No branches or pull requests

2 participants