add some helpful predicates #60
Conversation
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
==========================================
+ Coverage 93.41% 93.83% +0.42%
==========================================
Files 10 11 +1
Lines 395 422 +27
Branches 57 64 +7
==========================================
+ Hits 369 396 +27
Misses 15 15
Partials 11 11
Continue to review full report at Codecov.
|
codewatch/predicates.py
Outdated
| eg. | ||
|
|
||
| ``` | ||
| from my_models import Grade |
There was a problem hiding this comment.
Can we use a more generic example than Grade (here and throughout the project)?
There was a problem hiding this comment.
Outdated, but still an issue.
codewatch/predicates.py
Outdated
|
|
||
| class CallNodePredicates(object): | ||
| @staticmethod | ||
| def does_node_call_method(call_node, method_name): |
There was a problem hiding this comment.
This is tricky. 😓
Though we found use for it in that large codebase, I'm not sure we want to encourage use of this predicate widely. It has some limitations (being based solely on matching symbol names) that seem surprising and a bit under-powered to be a part of our standard interface.
There was a problem hiding this comment.
it's a bit under-powered but I think it's generally useful
There was a problem hiding this comment.
Co-Authored-By: lime-green <[email protected]>
|
|
||
| class CallNodePredicates(object): | ||
| @staticmethod | ||
| def has_expected_chain_name(call_node, method_name): |
There was a problem hiding this comment.
On the plus side, this new name is much better!
I'm still worried that this provides a seemingly-easy way to use codewatch (that we seem to want to encourage, by including it in the API) that won't actually do what people hope and will trigger subtle mistakes over time. My interpretation of why this is useful is that we've found inference so complex and unreliable that we're resorting to predicates by symbol names. This makes me unhappy.
Put another way, the biggest reason I don't like this is that it feels like giving up on the actual value of the AST approach. There's a not-so-tough regex that could be written to do this with grep. It's straight up textual-matching.
If we really want to include this, I'd be much happier if it was in a way that:
(a) made it more obvious that this should be an approach of last resort and
(b) included lots of warnings that this can easily fail to detect all kinds of different restatements of identical logic to the specified call chain.
There was a problem hiding this comment.
hmm ok. Yes it is indeed straight up text matching but it works alongside the rest of codewatch and is a good way to get started IMO
I'll include some warnings then
These predicates are meant to be general-use (call node) predicates to be used for visitor and inference-tip functions