search

fox-it / flow.record

Recordization library
GNU Affero General Public License v3.0
7 stars 9 forks source link

Make records sortable #108

Open JSCU-CNI opened 6 months ago

JSCU-CNI commented 6 months ago

This PR adds sorting functionality to the flow.record Record base class.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (09ed812) 80.20% compared to head (dd3dd5a) 80.21%. Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #108 +/- ## ========================================== + Coverage 80.20% 80.21% +0.01% ========================================== Files 33 33 Lines 3132 3134 +2 ========================================== + Hits 2512 2514 +2 Misses 620 620 ``` | [Flag](https://app.codecov.io/gh/fox-it/flow.record/pull/108/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/fox-it/flow.record/pull/108/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it) | `80.21% <100.00%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yunzheng commented 6 months ago

Sorting on _generated is universal across multiple different RecordDescriptors as this field should always exists, however i'm not sure if this is directly what people expect when you sort a collection of records. It could lead to unexpected behaviour.

Looking at dataclasses, it's not sortable by default either (unless you specify order=True, and then it would sort on the tuples), but then again it's not sortable across different dataclasses.

@Schamper do you have any opinions on this?

Schamper commented 6 months ago

Maybe another question first, what is the primary use-case for this feature? If it was the unit test of https://github.com/fox-it/dissect.target/pull/507, then this addition wouldn't have resolved that. Is there another reason why this should be added?

If we do add this, I think the dataclass approach sounds sane (only same type, maybe?, on tuples, yes). Which coincidentally would resolve that unit test issue.

JSCU-CNI commented 5 months ago

Maybe another question first, what is the primary use-case for this feature? If it was the unit test of https://github.com/fox-it/dissect.target/pull/507, then this addition wouldn't have resolved that. Is there another reason why this should be added?

The idea to sort records started with #507 so we could have predictable test output. I have no other reason to implement this (currently).

If we do add this, I think the dataclass approach sounds sane (only same type, maybe?, on tuples, yes). Which coincidentally would resolve that unit test issue.

I agree it would only be sane to make records with the same __class__ sortable. We can throw a TypeError if these are different between self and other. Would you recommend to sort a string output of Record._asdict() to mimic the dataclass implementation? Or something which more closely follows the _tuple_str function?