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

Add errors for PrefixList, PrefixMap, and Map with empty lists/dicts #1351

Merged
merged 5 commits into from
Nov 16, 2020

Conversation

aaronayres35
Copy link
Contributor

fixes #1191

PrefixList, previously if no default was given and the given list of strings was empty, had a default default value of None. In this PR, if the given list to PrefixList is empty an error is raised. Likewise, if Map or PrefixMap are fed empty dicts a ValueError is also raised. Previously, for Map and PrefixMap if an empty dictionary was given, it would raise a StopIteration exception. In this PR that is changed to a ValueError with a message explicitly specifying that the dictionary of valid values can not be empty.

Additionally, this PR adds trivial tests to ensure errors are raised when empty lists/dictionaries are used.

Checklist

  • Tests
    - [ ] Update API reference (docs/source/traits_api_reference)
    - [ ] Update User manual (docs/source/traits_user_manual)
    - [ ] Update type annotation hints in traits-stubs

else:
raise ValueError(
"The dictionary of valid values can not be empty."
)
Copy link
Member

Choose a reason for hiding this comment

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

You may want to make this a raise ... from None, so that we the KeyError doesn't show up in the traceback.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM, but consider changing the two raise statements to use raise(...) from None, to avoid the exception chaining in the traceback.

@mdickinson
Copy link
Member

LGTM! Waiting for CI, then I'll merge.

@aaronayres35 aaronayres35 merged commit 3e7da1f into master Nov 16, 2020
@aaronayres35 aaronayres35 deleted the fix-prefixlist-default-default branch November 16, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PrefixList default-default value is None?
2 participants