small improvements

[coveralls]

Coverage remained the same at 100.0% when pulling 7f32a8c7eca3d3f404de426cb52129d3bb0f54aa on psy:master into d5be2099749cb8adc967c44485a4ae11058ed9d5 on MartijnBraam:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f103ee28766ec7517bc517d28740c84837f1a97d on psy:master into d5be2099749cb8adc967c44485a4ae11058ed9d5 on MartijnBraam:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f103ee28766ec7517bc517d28740c84837f1a97d on psy:master into d5be2099749cb8adc967c44485a4ae11058ed9d5 on MartijnBraam:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f103ee28766ec7517bc517d28740c84837f1a97d on psy:master into d5be2099749cb8adc967c44485a4ae11058ed9d5 on MartijnBraam:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f103ee28766ec7517bc517d28740c84837f1a97d on psy:master into d5be2099749cb8adc967c44485a4ae11058ed9d5 on MartijnBraam:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 42d8360a31862752feaf9f87a8f807b8349f3199 on psy:master into d5be2099749cb8adc967c44485a4ae11058ed9d5 on MartijnBraam:master.

[MartijnBraam]

I don't think these are that great improvements. This patch deprecates one method that is guaranteed in use everywhere. Creates an memory cache that doesn't invalidate correctly and changes the function of get_valid into get_current.

If you absolutely don't want to parse the lease file twice you can always use the IscDhcpLeases.get() method to get all leases and filter them in your own application. The other methods are only some shortcuts for much used usecases.

Maybe @andir has some insights in this since he has some more high performance requirements.

[andir]

I would appreciate if we could have some kind of caching between the different filtered lists (active, valid, ..). Parsing my leases files already takes a lot of time and doing it twice isn't ideal.

Currently I've re-implemented the get_current method in my code since I'm generating several metrics & actions from the raw list. So caching would be one thing I'd like to see happen.

Regarding the deprecation of get(): The class has previously only been a container for the file that we are parsing and not much more. If you do not want to break current users code then we should maybe move to a different kind of architecture?

A simple methods retrieving all the leases would probably be sufficient. We could return a Result-DHCPLeases-Type which just holds all the parsed leases. The class could be sub-classed based on LeaseV4 or LeaseV6. On the Result-Type we could offer various "filters" like the currently provided "current". Also filters like "active" and maybe "inactive" would be of use.

I could imagine the user code to look like this:

leases = dhcp_leases.parse('/var/lib/dhcpd/leases.db')

type(leases) in [LeasesV4, LeasesV6]

# access properties providing lazily calculated lists of leases
leases.current
leases.active
leases.inactive

# some filters (just quick ideas so far)
leases.from_hardware('aa:bb:cc:dd:ee:ff')
leases.for('192.2.0.145')

# we could even make them chainable without much effort
l = leases.for('192.2.0.145')
l.valid
l.invalid
l.current

I've to admit that I was sitting next to @psy and was throwing some ideas onto him. I didn't really have motivation to do a lot of code-reviewing that day :-)

I'm willing to work on this (with whoever may be up for the task).

[MartijnBraam]

That looks like a neat api. If you break the api for new features then break it a lot for big improvements. The current api is design to just parse the file and get the raw data to process somewhere else. Some chainable filtering would be nice if you filter the data in the code directly.

That would probably be easily implementable. Every filter returns a new LeasesV4 or LeasesV6 object which also has the side effect of caching the data.

The seperate classes for v4 and v6 make it less easy to do get the ipv4 and ipv6 leases for the same mac address. Maybe just return a Leases class that can hold both v4 and v6 leases. Your example code doesn't show how the leases themselves look and how you retrieve them from the LeasesVx class. Maybe something like this:

>>> #Parse one or more lease files
>>> leases = dhcp_leases.parse(['/var/lib/dhcpd/leases.db', '/var/lib/dhcpd/leases6.db'])
>>> leases
<Leases ...>

>>> # get all active v4 leases
>>> leases.v4.active
<Leases ...>
>>> list(_)
[<LeaseV4 1.2.3.4>, <LeaseV4 2.3.4.5>]

>>> # get all addresses for mac
>>> leases.for(hardware='aa:bb:cc:dd:ee:ff')
<Leases ...>
>>> list(l)
[<LeaseV4 1.2.3.4>, <LeaseV6 fe80::1>]

This all will make it work more like a ORM thing for dhcp leases than a simple parser.

[andir]

I agree it adds a lot of complexity to the library. So why not start out by writing a new library that uses the current parsing lib and if we feel it is okay/sane/good/$metric merge it in as an addition while still keeping the old API ?

[andir]

I've written a simple example of what I thought would fit our ideas:

https://github.com/andir/isc-dhcp-filter/blob/master/isc_dhcp_filter/__init__.py

The tests show how it can be used: https://github.com/andir/isc-dhcp-filter/blob/master/tests/__init__.py

[andir]

In the mean-time I've made my hackup a proper library which can be installed via pip: https://github.com/andir/isc-dhcp-filter / https://pypi.python.org/pypi/isc-dhcp-filter/0.0.1