Skip to content

Conversation

@jonnytdevops
Copy link
Contributor

This is currently failing rubocop on the following:

lib/cisco_node_utils/snmp_notification_receiver.rb:24:5: C: Assignment Branch Condition size for initialize is too high. [41.61/40]
    def initialize(name,
    ^^^
lib/cisco_node_utils/snmp_notification_receiver.rb:24:5: C: Cyclomatic complexity for initialize is too high. [20/17]
    def initialize(name,
    ^^^
lib/cisco_node_utils/snmp_notification_receiver.rb:24:5: C: Perceived complexity for initialize is too high. [20/19]

However, this needs to be changed in the upstream rubocop code. My current code for this class is appropriate for the task, and is in line with other the other classes that has been merged.

@glennmatthews
Copy link
Member

You can change lib/.rubocop.yml to add the appropriate new upper bounds for these metrics as part of this PR.

Alternately, consider splitting the argument validation into a separate validation function - see snmpuser.rb for an example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using named args instead:

def initialize(name,
               instantiate: true, type: '', version: '', security: '', username: '',
               port: '', vrf: '', source_interface: '')

This will make the instantiation calls easier to read and less error prone:

    receiver = Cisco::SnmpNotificationReceiver.new(
      id,
      instantiate:      true,
      type:             'informs',
      version:          '3',
      security:         'priv',
      username:         'ab',
      port:             '45',
      vrf:              'red',
      source_interface: 'ethernet1/3')

@jonnytdevops
Copy link
Contributor Author

@glennmatthews @mikewiebe Can this please be merged, the code has been updated, thanks

@jonnytdevops jonnytdevops force-pushed the snmp_notification_receiver branch 2 times, most recently from ddd6067 to 0f2136a Compare December 18, 2015 13:05
CHANGELOG.md Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space between * and SNMP is causing the markdown to render this as italics instead of a bullet list - please fix.

@jonnytdevops jonnytdevops force-pushed the snmp_notification_receiver branch 2 times, most recently from 9c5e350 to cdfff68 Compare January 4, 2016 15:53
@jonnytdevops jonnytdevops force-pushed the snmp_notification_receiver branch from cdfff68 to fae258b Compare January 4, 2016 15:59
@jonnytdevops
Copy link
Contributor Author

@glennmatthews @chrisvanheuveln @mikewiebe Happy New Year! I've made the requested changes. It would be appreciated if this could be merged. Thanks

@glennmatthews
Copy link
Member

👍

mikewiebe added a commit that referenced this pull request Jan 5, 2016
@mikewiebe mikewiebe merged commit 08b0959 into cisco:develop Jan 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants