Conversation
Using `str.find()` to locate the colon denoting the end of the function definition fails in the presence of other annotations in the same line of source code, as it identifies the location of the first colon which isn't necessarily the close of the function definition. As this closing colon should be the last instance for a line of source code, changing to `str.rfind()` should mitigate this issue.
Following the repr string is much easier without having to go back & forth between the string & the class definitions.
The previous ReturnVisitor being used to identify whether or not a function returns anything other than None was flawed in its implementation, in that it visited all return nodes, even those in nested functions, and incorrectly used them in its determination about whether or not the top-level node returns something other than None. To fix this issue, a context switcher is implemented to keep track of the function node that contains the non-None return node. If node(s) are found, they are added to a set. The `has+only_none_returns` attribute is replaced by a property method that checks for the top-level node's presence in this set of nodes.
* By moving to a more generic visitor that keeps track of the node's context, we can more reliably discriminate between class methods and "regular" functions without needing to resort to manual iteration over child nodes. This also ends up greatly simplifying the entire visitor class as well. * Fix typo in nested class test case, which was resulting in a mismatch between the parsed function names and the specified truth values.
GhostofGoes
approved these changes
Feb 29, 2020
Contributor
GhostofGoes
left a comment
GhostofGoes
left a comment
There was a problem hiding this comment.
LTGM (and tests passed on Windows :))
lemonsaurus
approved these changes
Mar 1, 2020
Contributor
lemonsaurus
left a comment
lemonsaurus
left a comment
There was a problem hiding this comment.
Looks good. 👌
The context switching black magic is particularly clever.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
v2.0.1
Added
pep8-namingto linting toolchainblackcheck-merge-conflictcheck-tomlcheck-yamlend-of-file-fixermixed-line-endingpython-check-blanket-noqaChanged
ArgumentandFunction__repr__methods to make the string more helpful to readFixed
Nonereturning functions when they contained nested functions with non-Nonereturns (thanks @isidentical!)Additional Details
The previous approach being taken for nesting as to use a generic_visit() on the node to visit any nested functions. However, from the bugs reported (#67, #69) it was made clear that this approach was too generic & a method for either restricting or keeping track of the depth of this generic visit was required.
Both the
FunctionVisitorandReturnVisitorclasses have been rearchitected to keep track of the parent context of the nodes being visited in order to properly classify methods of nested classes and return values of nested functions, respectively.You can view a full writeup here on discuss.python.org.
Closes: #67
Closes: #69
Closes: #70
Closes: #71