A few minor fixes#224
Conversation
`defaults` is the default values e.g. `1` in `foo=1`. It's just used to find the offset of the kwargs, but the code reused the name which made me scratch my head for a minute.
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
==========================================
- Coverage 93.88% 93.08% -0.81%
==========================================
Files 9 14 +5
Lines 1129 1677 +548
Branches 21 117 +96
==========================================
+ Hits 1060 1561 +501
- Misses 66 101 +35
- Partials 3 15 +12
Continue to review full report at Codecov.
|
| hookimpl = HookimplMarker("example") | ||
|
|
||
|
|
||
| def test_uses_copy_of_methods(): |
There was a problem hiding this comment.
The test has a typo return instead of assert. If the typo is fixed, it fails. Actually the methods are not copied. Since I think the test is ill conceived and tests deprecated functionality, I opted to just remove it.
|
|
||
| # call with history; no results returned | ||
| pm.hook.myhook.call_historic(config=config, args=sys.argv, result_callback=callback) | ||
| pm.hook.myhook.call_historic( |
| res = getattr(method, self.project_name + "_impl", None) | ||
| except Exception: | ||
| res = {} | ||
| normalize_hookimpl_opts(res) |
There was a problem hiding this comment.
Hmm maybe we could do the same single conditional block like at line 111 at the end of this method?
There was a problem hiding this comment.
I can't figure out what "same single conditional block like at line 111" refers to, can you explain more?
There was a problem hiding this comment.
Ahh sorry my local version of the code is out of date. So basically a block like this at the end of this method which does the normalization if res != None. This might also mean we can remove line 116? A brief look shows this is the only place that method is used internally so I think we might be ok with that change.
@RonnyPfannschmidt @nicoddemus do we use that function in pytest for some reason?
There was a problem hiding this comment.
Doesn't look like it. @bluetech I think you should be fine to factor that call into parse_hookimpl_opts() and remove it from 116.
Probably makes sense to have normalization be atomic with the parsing steps but lets see what others think.
There was a problem hiding this comment.
There is a test test_parse_hookimpl_override which overrides parse_hookimpl_opts, returns {} and expects it to work. I did not consider this option so the premise of my commit was wrong. I removed it.
| if defaults: | ||
| index = -len(defaults) | ||
| args, defaults = args[:index], tuple(args[index:]) | ||
| args, kwargs = args[:index], tuple(args[index:]) |
There was a problem hiding this comment.
Agreed that this name is better.
The test passes invalid values to the instance (it does not takes functions, it takes HookImpls). The test fails, but it accidentally used `return` instead of `assert` so it wasn't visible. Since what is being tested is evidently unimportant and legacy, just remove it.
It doesn't take `**kwargs` but an argument `kwargs`.
goodboy
left a comment
There was a problem hiding this comment.
Looks fine to me. I am still interested if we can factor that normalization step into the parse method. Despite there being a test for overriding it I'm betting no client code is actually doing that. Might be worth creating a new issue to play with?
Please see the commit messages.