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

Optionally Allow Direct Passing of Values with EnumEnforcer #170

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

thomas-schweich
Copy link
Contributor

I decided to make a change to EnumEnforcer in the fork of this repository that I have been using, and figured I'd clean it up, add tests, and run it by you guys. Please consider this PR as a suggestion--I will not be offended if you'd rather not implement this feature, especially given that I did not consult with anyone about this change. Feel free to "take it or leave it".

I fully agree with the rationale behind storing the allowed API request parameters in Enums and having automatic usage enforcement. However, when using a Client in an in interactive console, I found it a bit annoying to have to keep switching between the official API docs and the tda-api docs to locate the correct Enums to use for every request parameter. The imports are also very verbose, and generally not very conducive to interactive usage.

As a result, I propose the following compromise: optionally allow direct passing of values using a new keyword for EnumEnforcer (and BaseClient) called allow_values which, when set to True, causes EnumEnforcer to allow the direct passage of enumerated values, as long as they are given as definitions in the relevant Enum subclass. It essentially turns EnumEnforcer into a spell-checker. I believe I have managed to implement this feature without breaking backwards compatibility, with the possible exception of EnumEnforcer.type_error (I'm not sure if this method was considered to be public facing--it was not documented) which, interestingly, had the effect of raising a ValueError.

I also noticed that the EnumEnforcer class did not have unit tests, so I went ahead and wrote them (accounting for this new keyword argument as well as original use cases). I added some new exceptions called UnrecognizedValueException (a ValueError subclass) and EnumRequiredException (which inherits from both TypeError and ValueError for backwards compatibility). EnumRequiredException is raised by EnumEnforcer when an invalid value is passed and enforce_enums is True but allow_values is False. UnrecognizedValueException is raised when an invalid value is passed while both are set to True.

There was also an issue with the ValueError EnumEnforcer raised--the description said to set enforce_enums to True to disable the parameter checking. I assume this was unintentional. I went ahead and changed the error messages to explain the two keywords and also link to this repository in case a valid value is missing from an Enum.

EnumEnforcer.convert_enum_iterable was also refactored to use repeated calls to convert_enum instead of implementing its own logic. I noticed that convert_enum_iterable was written such that it was acceptable to pass a single enum member, which would be converted to a list of one element. I kept this functionality for backwards compatibility, but only for when an enum member is passed--not when a value is passed. This is because an instance of str is both a valid value for some enums but also an iterable, so it makes more sense to just force the user to use a sequence of one element instead in this case.

Please let me know if this is of interest to the maintainers/community of this repo. Cheers!

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.

1 participant