Simplify the command type

[Miauwkeru]

Instead of changing the behaviour of the adapters to accomodate the command type like in #130
I opt instead to simplify the command type itself and make the structure completely flat.
If we ever want to change how it gets formatted for an adapter, it feels like this should happen at the adapter level
instead of the packing level

[codecov]

Codecov Report

❌ Patch coverage is 95.16129% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.25%. Comparing base (cd69cf1) to head (f1cbabf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
flow/record/fieldtypes/__init__.py	95.08%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   83.26%   83.25%   -0.02%     
==========================================
  Files          35       35              
  Lines        3705     3702       -3     
==========================================
- Hits         3085     3082       -3     
  Misses        620      620              

Flag	Coverage Δ
unittests	83.25% <95.16%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yunzheng]

Before I continue reviewing, it doesn't seem to pass these simple tests:

def test_empty_command() -> None:
    command = fieldtypes.command()
    assert command.executable == None
    assert command.args == None


def test_empty_string_command() -> None:
    command = fieldtypes.command("")
    assert command.executable == ""
    assert command.args == []

Would error with:

flow/record/fieldtypes/__init__.py:766: in __init__
    self.executable, self.args = self._split(self._raw)
                                 ^^^^^^^^^^^^^^^^^^^^^^
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = (executable=None, args=None), value = ''

    def _split(self, value: str) -> tuple[path, list[str]]:
>       executable, *args = shlex.split(value, posix=self._posix)
        ^^^^^^^^^^^^^^^^^
E       ValueError: not enough values to unpack (expected at least 1, got 0)

Also i'm not sure if my asserts would match with what one would expect. That is up for debate.

[Miauwkeru]

Before I continue reviewing, it doesn't seem to pass these simple tests:

Seems i forgot to add those simple tests, added them now

Also i'm not sure if my asserts would match with what one would expect. That is up for debate.

I don't think I'd expect everything to be None if i pass an empty string to it. executable and args are now filled up with "" and [] respectively

[yunzheng]

I made some slight changes.

There is one thing though, it's that command.args is mutable.
But when you mutate it and pack/serialize it again, it's not taken into account. So not sure if it should be a tuple or fix it that it works correctly.

[Schamper]

I made some slight changes.

There is one thing though, it's that command.args is mutable.

But when you mutate it and pack/serialize it again, it's not taken into account. So not sure if it should be a tuple or fix it that it works correctly.

Probably should be a tuple then?

[yunzheng]

I made some slight changes.
There is one thing though, it's that command.args is mutable.
But when you mutate it and pack/serialize it again, it's not taken into account. So not sure if it should be a tuple or fix it that it works correctly.

Probably should be a tuple then?

I think that would be better indeed. @Miauwkeru can you make it so?

[Miauwkeru]

I think that would be better indeed. @Miauwkeru can you make it so?

I changed it to a tuple

[yunzheng]

I think that would be better indeed. @Miauwkeru can you make it so?

I changed it to a tuple

Thanks, the attributes are still settable though, for instance you can do:

cmd.executable = "cat"
cmd.args = ("hello.txt",)

Which works, but does not serialize/deserialize properly when writing the record. Maybe it's also better to make executable and args read-only. Or make it work with serialization and deserialization.

[respondersGY]

Is this PR still in progress?

[yunzheng]

with the latest merged PR #200 we should be able to directly initialize path with the value to determine the correct path type. Then you can als get rid of __path_type.

[Miauwkeru]

Do you mean removing the whole __path_type in general? As in that case, there will be issues with for example hello_world.exe that will be interpreted differently depending on what system it gets run on.

If you just mean this specific case, this could still be an issue because the whole type can change from windows to posix path and vise versa. Tho I already have an idea on how to fix that specific issue.

[yunzheng]

ah wait I thought __path_type was a function to determine the path type.

[Miauwkeru]

ah, no. It is a type[path], so either a windows_path or a posix_path :)