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

Don't misuse KeyError for the custom names function. #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danthedeckie
Copy link
Owner

Description

See #94

Makes KeyError and stuff a bit more friendly for users using a custom names lookup.

If their names function throws a KeyError unexpectedly, it was getting caught incorrectly.

This expands the documentation & makes it so that people using a custom names resolver should deliberately only throw a simpleeval.NameNotDefined, other exceptions would be raised correctly.

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #126 (219de17) into master (50453b1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #126   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files           2        2           
  Lines        1041     1063   +22     
=======================================
+ Hits         1040     1062   +22     
  Misses          1        1           
Impacted Files Coverage Δ
simpleeval.py 99.66% <100.00%> (+<0.01%) ⬆️
test_simpleeval.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

The two default names that are provided are ``True`` and ``False``. So if you want to provide your own names, but want ``True`` and ``False`` to keep working, either provide them yourself, or ``.copy()`` and ``.update`` the ``DEFAULT_NAMES``. (See functions example above).
In general, when it attempts to find a variable by name, if it cannot find one,
then it will look in the ``functions`` for a function of that name. If you want your name handler
function to return a "I can't find that name!", then it should raise a ``simpleeval.NameNotDefined``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function to return a "I can't find that name!", then it should raise a ``simpleeval.NameNotDefined``
function to return an "I can't find that name!", then it should raise a ``simpleeval.NameNotDefined``

function to return a "I can't find that name!", then it should raise a ``simpleeval.NameNotDefined``
exception. Eg:

.. code-block:: python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REPL sessions like this should use pycon as the syntax highlighting.

Suggested change
.. code-block:: python
.. code-block:: pycon

>>> simple_eval('a + b', names=name_handler, functions={'b': 100})
121

(Note: in that example, putting a number directly into the functions dict was done just to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(Note: in that example, putting a number directly into the functions dict was done just to
(Note: in that example, putting a number directly into the ``functions`` dict was done just to

@@ -526,24 +526,30 @@ def _eval_name(self, node):
try:
# This happens at least for slicing
# This is a safe thing to do because it is impossible
# that there is a true exression assigning to none
# that there is a true expression assigning to none
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be open to me running the codebase through codespell and typos?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants