rust-pcap / pcap

Rust language pcap library
Apache License 2.0
598 stars 138 forks source link

Add Capture::dead_with_options constructor #245

Closed gmacon closed 1 year ago

gmacon commented 2 years ago

I realized that using a builder isn't actually a backwards-breaking change if I keep the existing method as a shortcut, so here it is.

Fixes #243

gmacon commented 2 years ago

We can set a default value for the linktype, but I didn't see an obvious default value. (I looked for a DLT_UNSPECIFIED or similar, but didn't find one.) What linktype do you want to be the default?

Stargateur commented 2 years ago

It was an open question, if there is none so be it.

Wojtek242 commented 1 year ago

I find the motivation behind this PR good and would like to see it make into the code base, but I find the implementation highly inconsistent with the rest of the existing API.

In particular:

I was contemplating having another Capture type to account for this, but that would also be inconsistent with the other captures as they all assume a NonNull pcap pointer.

Whilst ugly, I would prefer for consistency's sake if rather than having a builder, you implement only a single new method on Capture<Dead> called dead_with_precision. The current requirements do not justify breaking consistency.

Wojtek242 commented 1 year ago

Oh and if you do implement the changes, please make sure to rebase first to pick up the clippy lint fixes so that we can run the CI on your PR.

gmacon commented 1 year ago

OK, done.

Wojtek242 commented 1 year ago

And apologies for the extra work, but you might need to rebase again to avoid merge conflicts on the CHANGELOG.

Wojtek242 commented 1 year ago

Thanks!