pylintrc: re-enable too-many-arguments #941
Conversation
xmkg
force-pushed
the
enhancement/pylint-enable-too-many-arguments
branch
from
bcb845d to
5699e5a
Compare
| if minimal_mode: | ||
| filtered = self.minimise(filtered, minimal_mode) | ||
|
|
||
| if fmt not in self.SUMMARY_FORMATS: |
There was a problem hiding this comment.
This is no longer needed as we now enforce fmt to be one of the values in the SUPPORTED_SUMMARY_FORMATS
xmkg
force-pushed
the
enhancement/pylint-enable-too-many-arguments
branch
from
5699e5a to
050feaf
Compare
hotsos/client.py
Outdated
| plugin=None, max_level=2): | ||
| if plugin: | ||
| filtered = {plugin: self._summary[plugin]} | ||
| def get(self, params: OutputManagerParameters = OutputManagerParameters()): |
There was a problem hiding this comment.
Passing mutable types as param defaults is not a good idea imho for the simple reason that changes (however accidental) to the parameter made during one call of get() will persist to future calls to get(). It would be safer to have the caller pass it in rather than set a default.
There was a problem hiding this comment.
from dataclasses import dataclass
@dataclass
class MyOptions:
flag: bool = False
def foo(flag, opt: MyOptions = MyOptions()):
if flag:
opt.flag = True
else:
print(opt.flag)
foo(False)
foo(True)
foo(False)
produces
foo(False)
foo(True)
foo(False)
There was a problem hiding this comment.
That was an interesting gotcha, thanks Ed!
hotsos/core/host_helpers/network.py
Outdated
| @@ -105,7 +109,8 @@ def stats(self): | |||
| s = FileSearcher() | |||
| seqdef = SequenceSearchDef( | |||
| # match start of interface | |||
| start=SearchDef(IP_IFACE_NAME_TEMPLATE.format(self.name)), | |||
| start=SearchDef(IP_IFACE_NAME_TEMPLATE.format( | |||
| self.name)), | |||
xmkg
force-pushed
the
enhancement/pylint-enable-too-many-arguments
branch
3 times, most recently
from
cba6bf3 to
f6c9a7c
Compare
| ret = self.categorise_events( | ||
| event, | ||
| results=results, | ||
| options=type(self).EventProcessingOptions(key_by_date=False) |
There was a problem hiding this comment.
nit: it isn't necessary to use type() here?
since this does work either why i wont block on this and can fix in followup patch
There was a problem hiding this comment.
It's not necessary, it's just shorter to type type(self). instead of EventProcessingUtils.
There was a problem hiding this comment.
Also, I wanted to avoid using self. because it looks like a method call to me, which is not the case. But it might still be confused with a static or class method call, so I doubt the merit of doing that now.
There was a problem hiding this comment.
Replaced all with "self."
There was a problem hiding this comment.
this is why i dont like nested functions and classes ;)
There was a problem hiding this comment.
well, that and they are harder to test
hotsos/cli.py
Outdated
| force, event_tally_granularity, max_logrotate_depth, | ||
| max_parallel_tasks, list_plugins, machine_readable, output_path, | ||
| command_timeout, sos_unpack_dir, scenario, event, **kwargs): | ||
| def cli(*args, **kwargs): |
There was a problem hiding this comment.
I was concerned that doing this would make it more likely to harder to spot mistakes with ordering of args since they must respect the ordering of decorators but in fact it reduces the risk because the mapping is taken care of by python into the dataclass fields which is kindof nice.
hotsos/cli.py
Outdated
| @@ -188,7 +189,52 @@ def progress_spinner(show_spinner, path): | |||
| thread.join() | |||
|
|
|||
|
|
|||
| def main(): # pylint: disable=R0915 | |||
| def filter_unexpected_fields(cls): | |||
There was a problem hiding this comment.
it took me a few to understand why this is necessary so a docstring is needed here. can followup with a patch to add that.
There was a problem hiding this comment.
This method is now incorporated into the CLIArgs class with a docstring.
| sys.stdout.write('\n'.join(plugintools.PLUGINS.keys())) | ||
| sys.stdout.write('\n') | ||
| return | ||
|
|
||
| with progress_spinner(not quiet and not debug, drm.name): | ||
| with progress_spinner( | ||
| not arguments.quiet and not arguments.debug, drm.name): | ||
| plugins = [] |
There was a problem hiding this comment.
This is now unfortunately broken. we were using kwargs that were not explicitly defined i.e. from (**kwargs) as plugin names to be run and since almost everything is now in kwargs this now blows up when you use any on the command line. We therefore need to find a way to identify plugin names in kwargs - maybe could pull the nested new_init() function out of filter_unexpected_field() and generalise it to use for this purpose also.
There was a problem hiding this comment.
Good catch, I'll fix it shortly
xmkg
force-pushed
the
enhancement/pylint-enable-too-many-arguments
branch
2 times, most recently
from
57d8e6d to
b1c57e4
Compare
started using dataclass types for grouping long list of arguments. Suppressed the warning for test functions. Signed-off-by: Mustafa Kemal Gilor <[email protected]>
xmkg
force-pushed
the
enhancement/pylint-enable-too-many-arguments
branch
from
b1c57e4 to
141e069
Compare
started using dataclass types for grouping long list of arguments.
Suppressed the warning for test functions.