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

Move string parsing to separate function #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dholl
Copy link

@dholl dholl commented Feb 27, 2021

This pull request addresses issue #24 by moving the string parsing code into a separate function. It preserves existing API usage, but adds a new publicly-exposed function.

@dholl
Copy link
Author

dholl commented Feb 27, 2021

Comments / changes are welcome! I'm indifferent to exactly how #24 gets implemented, so in this first draft, I attempted the minimum possible to isolate the parsing logic. But maybe folks want this as a static function instead?

Personally, instead of returning a list of leases, I'd rather tweak this function into returning an iterable to permit parsing very large lease files and filtering for active leases on-the-fly. If folks are okay with this idea, I'm happy to tweak this new function and update the pull request. It would change the "simple case" calling from

leases = isc_leases_from_string(the_string)

to

leases = list(isc_leases_from_string(the_string))

It would also allow for things like:

active_leases = list(filter(lambda lease: lease.valid and lease.active, isc_leases_from_string(the_string)))

or fancier:

for active_lease in filter(lambda lease: lease.valid and lease.active, isc_leases_from_string(the_string)):
    # do something like launch coroutine to investigate this lease...

but the basic idea is that for very large lease files, we never have to have an entire list of Lease objects in memory at once if we don't need it, especially since the contents of the lease file is still loaded as a string.

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.

None yet

1 participant