-
Notifications
You must be signed in to change notification settings - Fork 9
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 empty value in path fieldtype #122
Conversation
Currently |
No idea why Python 3.12 fails, will look into it later |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
+ Coverage 83.67% 83.72% +0.04%
==========================================
Files 34 34
Lines 3394 3403 +9
==========================================
+ Hits 2840 2849 +9
Misses 554 554
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
assert p1._empty_path | ||
assert str(p1) == "" | ||
assert p1 != path_cls(".") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your solution is somewhat similar to my original proposal, which was rejected because it failed the following test (which also fails here):
# initialize with empty path
p1 = path_cls(Path(""))
assert p1 == ""
assert p1._empty_path
assert str(p1) == ""
assert p1 != path_cls(".")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is because you initialise a flow.record path with a stdlib Path("")
. I think the original PR was rejected because we don't want to change the behaviour of pathlib.Path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my information this was part of the requirements. I see no other way to implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as this works with our own TargetPath
, all should be well in the world. And even then, we generally just directly call windows_path(variable)
anyway. Having side-effects is a big blocker, so if that's really the only way, then we should just document this behaviour and force usage through this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was not much information regarding the requirements in the DIS ticket, other than that Record path fields do not support empty values as they would end up as the .
dot value. This PR fixes that.
If windows_path
and posix_path
fields are intitialised using TargetPath
variables, then we need to ensure that TargetPath
also supports empty paths and then we can test if that works.
Normally an empty path would be normalized to a "." (dot) character. This change allows you to initialize a path field with an empty string. This is useful to represent a path that is empty. Fixes DIS-2557
e5f6480
to
d5f8264
Compare
Normally an empty path would be normalized to a "." (dot) character.
This change allows you to initialize a path field with an empty string.
This is useful to represent a path that is empty.
Fixes DIS-2557