Add type checks for initialiazion of models #1144
Conversation
|
Looks good. Did you run |
praw/models/reddit/redditor.py
Outdated
| elif fullname: | ||
| self._fullname = fullname | ||
| if not isinstance(fullname, str): | ||
| raise TypeError( | ||
| "The id must be type `str`." |
There was a problem hiding this comment.
Message should say fullname rather than id
There was a problem hiding this comment.
fixed it now, thanks
Update commits from master
caps error
|
I am completely overhauling this part of the code, so I will close the PR until my features are finished. |
Update changes
Update PR
|
I decided to add in a new feature decided to simplify the type checking process. It lives in praw/models/utils. Suggestions to move it anywhere? |
Moving stuff to ../util/
Moved code
Moving code
finish move
|
If you have any idea on how to make the function smaller, as it has a lot of if statements to cover every possibility of misuse by end-users. Although, end users are most likely not going to use this function, so I can remove the majority of the checks, such as for using None as an expected type. |
|
@PokestarFan I appreciate all the effort you're putting into these changes. However, I worry it might be a bit overkill to type check every parameter. If someone wants that level of validation a dynamically typed language probably isn't for them. A better use of effort would be to use python's type hinting which is available in python 3.5+, which conveniently is what we support. Thoughts? |
|
@bboe should I add in type hinting through stub files, or directly into the functions/classes? |
|
For now I am going to only add in type checks to the public reddit methods such as submission. |
Pull from upstream
|
@bboe @jackodsteel I have finished this PR. Every method that can be called from the reddit instance without accessing any attributes (such as Redditor, Comment, Submission, etc.) have been type checked. I did not implement type-checking onto any of the inbox methods, as those are methods of a class, not a callable class on the Reddit instance. |
|
@PokestarFan I think I may not have been clear in my previous message. I don't see a ton of value in introducing explicit runtime type checks. Most issues should surface pretty quickly at runtime anyway if the wrong type is provided without said checks. While I understand that the exceptions as-is may be a little cryptic for people, the added complexity (2000+ lines for this PR, which is only a subset of methods) doesn't seem worth it. I think parameter type-hinting could help considerably for said cases and it can be done with relatively little effort. Aside from that, the places where there exist some run-time checks are when parameters are mutually exclusive, or parameters must be grouped together. One could argue that in such cases, we might be better off making separate methods. While that's certainly a possibility, I haven't really heard any complaints about how it's currently done. |
|
Understood. I will look into type-hinting, and work on mutually exclusive methods. |
|
To anyone reading this later, I moved the code into PokestarFan/validate-types and force-pushed master to resemble master of praw |
Fixes # (provide issue number of applicable)
#1143: Adds in type checks to try and correct the errors being caused in the issue