Skip to content
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

Feature/iprange enhancements #252

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Feature/iprange enhancements #252

wants to merge 6 commits into from

Conversation

ryanbreed
Copy link

@ryanbreed ryanbreed commented Oct 9, 2016

The #include? method now can accept Strings, IPAddr, and Nexpose::IPRange objects when testing for inclusion.

Description

Comparisons have been delegated to private methods #include_iprange? and include_ipaddr? (which passes through to include_iprange?). Care has been taken to preserve old behavior and trap runtime exceptions for compared strings that cannot be coerced into a compatible type. The method will raise a Nexpose::IPRange::IncompatibleType if an attempt is made to compare with an instance that cannot be delegated to a private implementation-specific comparison.

Motivation and Context

This enhancement will behave more correctly in more obvious ways for comparisons to commonly used data types. IPRange has also been extracted from site.rb into its own separate file.

  1. Fixes 240
  2. Supersedes PR 249 - see discussion there for further context.

How Has This Been Tested?

  • Existing unit tests pass
  • New unit tests have been extracted to spec/nexpose/ip_range_spec.rb:
    Finished in 0.05846 seconds (files took 0.52899 seconds to load)
    93 examples, 0 failures

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have updated the documentation accordingly (if changes are required).
  • I have added tests to cover my changes.
  • All new and existing tests passed.

adding private method implementations
extracting Nexpose::IPRange and moving spec up
mark argument-specific include? implementations as private
fixing invalid type tests
remove test due to class initialization bug
@ryanbreed
Copy link
Author

@gschneider-r7 @sgreen-r7 - ok here's a cleaned up PR with more concise/consistent unit tests and rubocop style. I can tack on another commit if you'd like me to clean up the hound violations as well.

I'll do another PR if you're interested in a refactor of the class implementation. Otherwise, I'd like to start working on specs to get the code coverage up.

@gschneider-r7
Copy link
Contributor

I'm not too concerned about the Hound comments on this PR. I'll need to set aside some time with @sgreen-r7 to review the open PRs and get some stuff merged soon hopefully.

Thanks again for working on this!

@sgreen-r7
Copy link
Contributor

@ryanbreed - Gavin and I looked at this PR for a bit yesterday, and there a few things that don't fit within our "style". However your PR gave us a few really good ideas! I'm going to work on refactoring a bit, and update your PR tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPRange include? function error
3 participants