FIX: correct path semantics behavior #703
Conversation
|
Before making it ready for review, it will be tested with the following scenarios:
|
|
Looks like this could be a good step in the right direction! 👍 |
|
You may want to see this @danielballan windows path fix too |
| """ | ||
| 'a/b/c' -> 'a/b/c/' | ||
|
|
||
| EPICS adds the trailing slash itself if we do not, so in order for the | ||
| setpoint filepath to match the readback filepath, we need to add the | ||
| trailing slash ourselves. | ||
| """ | ||
| return os.path.join(path, '') | ||
| if path_semantics is 'posix': |
There was a problem hiding this comment.
This should check equality (==) not identity (is). It happens that this works, but we should not rely on it. See for example this StackOverflow answer.
| if path_semantics is 'posix': | ||
| return f'{PurePosixPath(path)}/' | ||
| else: | ||
| return f'{PurePath(path)}{os.path.sep}' |
There was a problem hiding this comment.
Consider the scenario where ophyd is running on a Linux workstation but the area detector driver is running on Windows, as is the case at some beamlines. In that situation, os.path.sep will return / but the driver needs to be set to \. I suggest something along the lines of:
if path_semantics == 'posix':
return f'{PurePosixPath(path)}/'
elif path_semantics == 'windows':
return f'{PurePath(path)}\'
elif path_semantics is None:
# We are forced to guess which path semantics to use.
# Guess that the AD driver is running on the same OS as this client.
return f'{PurePath(path)}{os.path.sep}'
else:
# This should never happen, but just for the sake of future-proofing....
raise ValueError(f"Cannot handle path_semantics={path_semantics}")
There was a problem hiding this comment.
@danielballan, I like the more explicit conditions.
For the Windows part it should be either f'{PurePath(path)}\\\\' or fr'{PurePath(path)}\\' to have \\ returned at the end. Also, I think it should better be PureWindowsPath.
There was a problem hiding this comment.
I played with it a bit, and f'{PurePath(path)}\\' should be OK (we need two backslashes to escape a special backslash symbol).
|
I added the suggested improvement and expanded it to be used in the |
mrakitin
force-pushed
the
fix_path_semantics
branch
from
d916b18
to
3bd4cf8
Compare
|
This languished for far too long! Sorry, @mrakitin, @EliotGann, and @RussBerg. This will go out in ophyd v1.4.1 on Monday. |
|
Thank you, @danielballan. |
|
Just one year? Seems about right... |
|
As a Russian proverb says, "one has to wait for three years for what is promised" 😄 |
This is found with @mrakitin at NSLS-II RSOXS beamline when the collection is done on the Windows machine, and the detector is on a Linux one.