-
Notifications
You must be signed in to change notification settings - Fork 11
Linter: enable attribute-defined-outside-init rule #528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| # Make sure that empty bodies like b"" don't get sent. | ||
| return None | ||
| if isinstance(body, bytes): | ||
| body = body.decode("utf-8") # Decode byte input to string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_body_object reassigns parameter 'body' (e.g., body = body.decode(...)). Assign decode result to a new local variable to preserve the original argument.
Details
β¨ AI Reasoning
βA newly added static method transforms its input parameter by decoding bytes into a string and reassigning the same local parameter name. Reassigning a function parameter can obscure the original value and its type; here the method both decodes and later parses the same parameter, which may confuse readers and complicate debugging. The pattern is normalization (decoding) before use, but reassigning the parameter rather than assigning to a new local variable reduces clarity.
π§ How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| """ | ||
|
|
||
| def __init__(self): | ||
| self.config = ServiceConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialization logic for config/routes/middleware_installed is duplicated between init and reset
Details
β¨ AI Reasoning
ββ1) The diff adds explicit initializations in init for config, middleware_installed, and routes (lines 20-23) while reset() also sets config and clears routes (lines 41-42); 2) This duplicates the same initialization logic in two places, increasing maintenance burden because changes must be kept in sync; 3) This harms maintainability by introducing obvious within-file duplication that can lead to inconsistencies; 4) The violation was introduced by this PR (previously init delegated initialization to reset), so it's new; 5) The duplication is straightforward to refactor (extract common init helper or have init call reset); 6) Fixable within this PR scope.
π§ How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| connection_manager.conf.set_blocked_user_ids(res.get("blockedUserIds", [])) | ||
| connection_manager.conf.set_bypassed_ips(res.get("allowedIPAddresses", [])) | ||
| if res.get("receivedAnyStats", True): | ||
| connection_manager.conf.enable_received_any_stats() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag doesn't update like other settings but seems fine, will only be false for the first heartbeat (and this flag is deprecated anyway). Needs to start sending heartbeat after 30s, then 2 minutes, then every 10 minutes
| self.bypassed_ips = IPMatcher() | ||
| self.blocked_uids = set() | ||
| self.last_updated_at = -1 | ||
| self.received_any_stats = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was previously true fyi
Co-authored-by: Hans Ott <hansott@hotmail.be>
| self.body = Context.parse_body_object(body) | ||
|
|
||
| @staticmethod | ||
| def parse_body_object(body): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method docstring restates what parse_body_object does; explain why these parsing rules exist (e.g., server quirks, expected input shapes) instead of repeating the mechanics.
Details
β¨ AI Reasoning
βThe new staticmethod parse_body_object has a docstring that repeats the mechanics implemented in the method rather than providing rationale or design decisions. It doesn't explain why empty byte bodies should become None, why JSON is heuristically detected, or compatibility constraints that motivated this behavior.
π§ How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Summary by Aikido
β‘ Enhancements
π§ Refactors
More info