Add Flake8-BugBear and Bandit (Security!) #637
Conversation
|
The missing asserts are embarrassing. Not convinced by the raises and the stacklevels |
This is why I like this ! haha
I can #noqa these changes :) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
- Coverage 91.62% 91.58% -0.05%
==========================================
Files 22 22
Lines 2280 2281 +1
==========================================
Hits 2089 2089
- Misses 191 192 +1 ☔ View full report in Codecov by Sentry. |
The raises are very opinionated, it looked like the code was specifically looking for the exception and converting it to something else for the user. I don't know of noqa makes more sense than disabling altogether The stacklevel I simply don't get. Isn't 1 the default? |
|
Also for the zip, is it forcing a default or asking you to be explicit? |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Asking to be explicit
I do not have a strong opinion here. Maybe we can simply ignore that rule.
The stacklevel needs to be explicit to I am putting the default. Qw can #noqa these ones. WDYT? |
juanitorduz
changed the title
It might be the default, but isn't it recommended to have at least 2 or more? else nothing shows up? |
Think I am the culprit for a few a these 🙈 |
Copilot suggests 2 XD so we can use that and not the default hehe |
This is why I like this plugin, I did it many times and probably will. So I need a system to prevent this haha |
|
the level depends on what you want the user to see as the source of the warning. It's usually one, but sometimes it's called by a helper or a very internal function and 2 (or 3 or whatever) is more appropriate. I don't think 2 is recommended blindly? |
|
So then we can take it case by case. This is good imo to have to think about this :) |
I suggest we leave |
|
Personally, I like to know where the errors are coming from so I am in for keeping the |
Some errors are being expected/ handled by us, so there's not much to gain from passing the original error. No strong opinion though. In this example, I don't see what's the point of raising the original exception. The user may have no idea about how PyMC model contexts work, and I would guess the whole trace-back would be more confusing than our curated error. def user_facing_foo():
# This function must be called in a model context.
try:
pm.model_context(None)
except ModelContextError: # or whatever
raise RuntimeError("You must call this function from within a PyMC model") |
|
Yep! That makes sense! Then let's revert and ignore this rule and make sure we think about the user when reporting errors (as we have been doing) 🙌 |
|
@ricardoV94 done ✅ |
|
Btw, my last comments are mostly nitpicks. Just wanted to emphasize those assert is not None were by design not errors to avoid code duplication, and needed only because mypy is too retarded to analyze type flow across function calls. Basically the situation was @property
def fit_result(self):
if self.idata is None or "posterior" not in idata.groups: # or whatever
raise ValueError(...)
return self.idata.posterior
def foo(self):
# Raise informative error
self.fit_result
# Use self.idata (or self.fit_result, which mypy would not have complained abuot I guess)
self.idata... # mypy complains it may be None |
|
Thanks for the feedback and comments @ricardoV94 ! Could you please see if you agree with the changes 🙏 ? |
Co-authored-by: Ricardo Vieira <[email protected]>
|
@ricardoV94 It was my first time using the |
Me too haven't found a good use for it yet |
|
Thank you @wd60622 and @ricardoV94 🎸 ! |
Let me know what you think about it :)
📚 Documentation preview 📚: https://pymc-marketing--637.org.readthedocs.build/en/637/