Fix expand parameters bug in util to support any parameters#6
Fix expand parameters bug in util to support any parameters#6vishrutshah merged 4 commits intomonitor-clifrom
Conversation
Adding --filters into activity-logs & tenant-activity-logs Adding --ids support into alert-rules & autoscale-settings' show and delete commands
| # pylint: disable=too-many-arguments | ||
| def list_activity_logs(client, correlation_id=None, resource_group=None, resource_id=None, | ||
| resource_provider=None, start_time=None, end_time=None, | ||
| # pylint: disable=too-many-arguments, line-too-long |
There was a problem hiding this comment.
The line-too-long is for which line. Can we overcome it?
There was a problem hiding this comment.
It's coming from line 132
if len([x for x in [correlation_id, resource_group, resource_id, resource_provider] if x]) > 1:
if i split it i think it'd be not readable.
Or i can create local list for [correlation_id, resource_group, resource_id, resource_provider] to decrease characters on line 132
There was a problem hiding this comment.
I saw this pattern been adopted in multiple places. Why not extract it into a method? like
def single(collection):
return len([x for x in collection if x]) > 1There was a problem hiding this comment.
sounds great. Updated it.
| type=json.loads, | ||
| help='JSON encoded condition configuration. Use @{file} to load from a file.') | ||
|
|
||
| for command in ['show', 'delete']: |
There was a problem hiding this comment.
I don't appreciate this form of registering command parameters, and it is the very form of registering I was trying to avoid when designing these patterns. The problem with using a loop to register parameters for multiple commands is sacrificing maintainability, readability for merely limited reusability benefits. It makes it difficult to locate where are the parameter registrations are. It requires extra effort when a future change is going to make to only one of the command. It breaks the pattern that parameters associating to one command are cluster together under one indented context. I would advise against this form of parameter registering and prefer duplication.
There was a problem hiding this comment.
hahah... Let me split it for simplicity.
4 participants
Adding --filters into activity-logs & tenant-activity-logs
Adding --ids support into alert-rules & autoscale-settings' show and delete commands
@troydai @tjprescott @derekbekoe Please review the PR when you get a chance. Thanks!