buildcache push: ensure bool arguments for include_*#35632
buildcache push: ensure bool arguments for include_*#35632haampie merged 4 commits intospack:developfrom
Conversation
The idea was to make arguments explicit, but unfortunately we now passed
```
push(include_root={"include_root": True, "include_dependencies": False})
```
| if type(include_root) != bool or type(include_dependencies) != bool: | ||
| raise ValueError("Expected include_root/include_dependencies to be True/False") |
There was a problem hiding this comment.
Curious if we would have caught this with an annotated function, using mypy.
There was a problem hiding this comment.
is this needed at all now that we have annotations and mypy? Or is this just a belt and suspenders thing after all that CI chaos?
There was a problem hiding this comment.
One other thought: I'm pretty positive this would've been caught if we checked the whole codebase with mypy. Right now we only check annotated functions. I think we should try to get to checking everything to prevent subtle stuff like this -- the codebase is large enough that it can save us a lot of pain.
Specifically: I think we should get to where we can turn on --check-untyped-defs
There was a problem hiding this comment.
This should prob use isinstance instead of type
scottwittenburg
left a comment
scottwittenburg
left a comment
There was a problem hiding this comment.
Yes, this makes sense, thanks @haampie! What a relief to have sanity restored.
Fixes a bug introduced in 44ed0de where the push method of binary_distribution now takes named args include_root and include_depedencies, to avoid the **kwarg hole. But the call site wasn't update and we passed a dict of keys/values instead of arguments, which resulted in a call like this: ``` push(include_root={"include_root": True, "include_dependencies": False}) ``` This commit fixes that, and adds a test to see if we push the correct packages.
5 participants
bindist.push(...)args were not correctly forwarded after a small API changethat should've made the arguments more explicit. Instead it caused dependencies
to be pushed too, where just the root node should've been.