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

simpleeval raises NameNotDefined exception when KeyError exception occured using callable names resolver #94

Closed
chedv opened this issue Sep 16, 2021 · 5 comments
Assignees
Labels
next release probably fixed Probably Fixed, waiting for confirmation
Milestone

Comments

@chedv
Copy link

chedv commented Sep 16, 2021

Here is the simplest example:

from simpleeval import SimpleEval

def variables_resolver(node):
    raise KeyError

expr_evaluator = SimpleEval()
expr_evaluator.names = variables_resolver
expr_evaluator.eval("test")

Output:

simpleeval.NameNotDefined: 'test' is not defined for expression 'test'

But if I use, for example, Exception instead of KeyError, I have only raised Exception. That's because of the following logic in the library:

def _eval_name(self, node):
    try:
        if hasattr(self.names, '__getitem__'):
            return self.names[node.id]
        elif callable(self.names):
            return self.names(node)
        else:
            raise InvalidExpression('Trying to use name (variable) "{0}"'
                                    ' when no "names" defined for'
                                    ' evaluator'.format(node.id))
    except KeyError:
        if node.id in self.functions:
            return self.functions[node.id]

        raise NameNotDefined(node.id, self.expr)

try ... except KeyError block is used on the entire function instead of just handling return self.names[node.id]

@danthedeckie
Copy link
Owner

That's a good point! Good catch, @chedv !

@danthedeckie
Copy link
Owner

So I think when I wrote it - I was expecting that if you write a custom resolver function, you use a 'KeyError' to say 'Not a valid name - but hey simpleeval - go check functions. Maybe they can help' kind of thing.

Which is ... not really defined behaviour.

Maybe a couple of custom exceptions would be the better way to do this?

So if the custom resolver raises NameNotDefinedCheckFunctions - then it would go on to check in the functions as before - but if it raises NameNotDefined - then it bypasses it, and any other exception gets raises directly...

@chedv
Copy link
Author

chedv commented Jan 16, 2022

Thanks for your response, @danthedeckie. When I apply functions to resolve variable names, I may use a complicated implementation with my own dictionaries and if I face a bug in my code that causes KeyError, it will confuse me with the NameNotDefined message instead of the root cause. Moreover, if it confuses end users, it will be much worse.

So I agree that a custom exception would be better instead of using standard Python exceptions. But also it would be good to mention the feature in the documentation.

@danthedeckie
Copy link
Owner

See the betters-names-exceptions branch...

@danthedeckie
Copy link
Owner

This should be fixed in 1.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release probably fixed Probably Fixed, waiting for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants