Redo variable and function names reserved for Python #165
Conversation
Changed the conflicting variable name `filter` and function name `filter()` accordingly which suricata-update uses as it is reserved for use in Python standard modules.
| @@ -252,7 +252,7 @@ def __init__(self, matcher, pattern, repl): | |||
| def match(self, rule): | |||
| return self.matcher.match(rule) | |||
|
|
|||
| def filter(self, rule): | |||
| def modify_filter(self, rule): | |||
There was a problem hiding this comment.
Repeating the functionality of a class in its methods is usually a bad practice. What I mean is, when you want to access this function, you'd probably do ModifyRuleFilter.modify_filter() see the modify twice? Was it even needed? Probably not. Because in this case filter has been defined under a separate namespace. Hence, ModifyRuleFilter.filter() makes a better candidate. However, since filter() is reserved, we should in any circumstances avoid using it directly. The next best thing would be _filter(). But, it is used to indicate private use methods.
Conclusion: I think something should be changed here, I can't think of a good function name. Please find something?
Also, if you think otherwise or find some guide/docs that provide a clear picture of naming scheme in such cases, please let me know.
There was a problem hiding this comment.
Hey @shivan1b I tried going through many guide/docs for such cases but couldn't find one. I could come up with a few names for the filter() function like filterRule(), drop()/modify(), dropRule()/modifyRule()and filtered(). Do you think any of these could be used?
There was a problem hiding this comment.
Hmm I guess then we should leave this as-is. Let @jasonish decide if this looks OK.
There was a problem hiding this comment.
So part of the idea is that a filter follows an interface, so executing doesn't require knowledge of what its doing. I'd prefer to rename the method to something more generic.. run(), or execute() seem to be common names in other languages where filters are part of a processing pipeline.
There was a problem hiding this comment.
Thanks for clarifying @jasonish. Will fix it in a new PR.
| @@ -291,7 +291,7 @@ def match(self, rule): | |||
| return False | |||
| return self.matcher.match(rule) | |||
|
|
|||
| def filter(self, rule): | |||
| def drop_filter(self, rule): | |||
|
Created a new PR #174. |
Changed the conflicting variable name
filterand function namefilter()which suricata-update uses as it is reserved for use inPython standard modules.
Make sure these boxes are signed before submitting your Pull Request
-- thank you.
https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
contribution agreement at
https://suricata-ids.org/about/contribution-agreement/
changes made (if applicable)
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/3012
Describe changes:
filterinmain.pyfilterintest_main.py