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

Add support for setting the network associated with the record #56

Merged
merged 14 commits into from
Sep 18, 2019

Conversation

oschwald
Copy link
Member

@oschwald oschwald commented Sep 17, 2019

When deserializing the object.

This also enhances the tests around getting the prefix length and includes some general code clean-up/modernization.

After doing many runs of the included benchmark, the before and after differences do not appear to be statistically significant. There were some small performance improvements, but then there is the cost of creating the Network object, passing it around, checking if the constructor param needs to be set, etc.

@@ -0,0 +1,135 @@
#region
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a class used in the deserialization tests. It is the same as TypeHolder except without the new network fields. I did this so that I didn't have to make significant changes to the database enumeration tests.

Copy link
Contributor

@horgh horgh left a comment

Choose a reason for hiding this comment

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

Looks great! Should we update the changelog too?

Would it be worth adding an example of using this to the readme do you think?

@oschwald oschwald force-pushed the greg/network-fixes branch 2 times, most recently from c0f2f1b to e1a9ca6 Compare September 18, 2019 17:18
@horgh horgh merged commit b1b6d99 into master Sep 18, 2019
@horgh horgh deleted the greg/network-fixes branch September 18, 2019 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants