Fixes #906 (BAN-B101) Assert statement used outside of tests #907
Conversation
lib/carbon/storage.py
Outdated
| @@ -136,10 +136,12 @@ def loadAggregationSchemas(): | |||
| try: | |||
| if xFilesFactor is not None: | |||
| xFilesFactor = float(xFilesFactor) | |||
| assert 0 <= xFilesFactor <= 1 | |||
| if not (0 <= xFilesFactor <= 1): | |||
There was a problem hiding this comment.
Unnecessary parentheses after keyword PYL-C0325 https://deepsource.io/gh/graphite-project/carbon/run/45eefeec-464d-4e8b-987c-127ceaaa7f4a/python/PYL-C0325
lib/carbon/storage.py
Outdated
| if aggregationMethod is not None: | ||
| if state.database is not None: | ||
| assert aggregationMethod in state.database.aggregationMethods | ||
| if not (aggregationMethod in state.database.aggregationMethods): |
There was a problem hiding this comment.
Unnecessary parentheses after keyword PYL-C0325 https://deepsource.io/gh/graphite-project/carbon/run/45eefeec-464d-4e8b-987c-127ceaaa7f4a/python/PYL-C0325
|
Hi @rohankhanna Could you please fix PYL-C0325 introduced in your change? Thanks! |
|
@deniszh Is there anything else I should do? Is this good to merge? |
lib/carbon/hashing.py
Outdated
| @@ -89,7 +89,8 @@ def remove_node(self, key): | |||
| self.ring_len = len(self.ring) | |||
|
|
|||
| def get_node(self, key): | |||
| assert self.ring | |||
| if not self.ring: | |||
| raise AssertionError("self.ring not set") | |||
There was a problem hiding this comment.
I'd move this check to the end of the constructor. No point checking this on every get_node(). The whole idea of assertions is that they are optimised away in production code but you can use it while developing/debugging.
There was a problem hiding this comment.
I didn't realize that we were deliberately trying to optimise away assertions.
There was a problem hiding this comment.
This code is 10 years old, no clue what the intention was. I also have no clue why Deepsource thinks having assertions in code outside of tests is a bad thing. I personally don't use them but there are many reasons one might want to, there is many CS research on that topic.
Anyway this looks good to me.
There was a problem hiding this comment.
Ah, but now tests fail... Aaargh
There was a problem hiding this comment.
We can probably just remove the whole check, it is kinda pointless, if ring = None then the bisect_left() fails anyway.
No description provided.