search

fox-it / flow.record

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

Empty path str() should not return cwd #120

Closed cecinestpasunepipe closed 3 months ago

cecinestpasunepipe commented 4 months ago

(DIS-2557)

codecov-commenter commented 4 months ago

Codecov Report

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

Project coverage is 81.73%. Comparing base (4a47670) to head (c8a46a1).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #120 +/- ## ========================================== + Coverage 81.64% 81.73% +0.09% ========================================== Files 34 34 Lines 3307 3324 +17 ========================================== + Hits 2700 2717 +17 Misses 607 607 ``` | [Flag](https://app.codecov.io/gh/fox-it/flow.record/pull/120/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/120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it) | `81.73% <100.00%> (+0.09%)` | :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.

Schamper commented 4 months ago

Do you have any examples where this is an issue and is considered a bug? Maybe in most cases if you encounter an empty string as a path value, setting the record value to None would be better.

There can be cases where we parse some artefact which may or may not have a path field. IMO there's a difference between the absence of the path field (None) or an empty path field ("") and thus it should be possible to be noted as such in the record.

yunzheng commented 4 months ago

My take on this issue: https://github.com/fox-it/flow.record/pull/122

As this is a totally different way to tackle this problem from this PR i've created a new branch and PR.

cecinestpasunepipe commented 3 months ago

My take on this issue: #122

As this is a totally different way to tackle this problem from this PR i've created a new branch and PR.

I added some comments to your PR. I like your solution and I was thinking along the same lines, however like I pointed out in my other comment, it does not seem to meet the requested requirements.

yunzheng commented 3 months ago

I left a comment in the first unresolved thread with my point of view, my other suggestions are still unresolved. If you want me to review anything else let me know.

yunzheng commented 3 months ago

After some discussion, this PR is closed in favour of https://github.com/fox-it/flow.record/pull/122