os: fix os.join_path('/', 'file.txt') and os.join_path_single('/', 'file.txt') (fix #20231)
#21429
Conversation
spytheman
changed the title
os.join_path(/, file.txt) and os.join_path_single(/, file.txt) (fix #20231)os.join_path('/', 'file.txt') and os.join_path_single('/', 'file.txt') (fix #20231)
|
@spytheman what are you doing? are you stealing my PR? Let's not waste double efforts into solving the SAME ISSUE on multiple fronts there are 850+ issues to work on why not work together?
|
…and os.join_path_single, fix a/./b.txt -> a/b.txt without allocations
I am fixing it in a different way, that will require way less edits and review changes. I am sorry, but |
|
Once it passes the CI, I can push to your PR, and then you will get the credit, when it is merged. I do not care about that at all. I simply do not want problems with the bootstrap and I have enough experience to know that changes in that particular place are tricky. |
…edge cases; add more tests
|
@hholst80, the CI passed for this PR, and imho it will be a bit less breaking, while ensuring the future stability of the behavior of os.join_path and os.join_path_single (which are now more unified in their output, and better tested). Do you want me to push the changes here to your PR, so that I can merge them there? |
| os.setenv('USERPROFILE', '/tmp/home/folder', true) | ||
| os.setenv('USERPROFILE', r'\tmp\home\folder', true) |
There was a problem hiding this comment.
If I did this I am fairly sure the word "regression" would come up. ;-)
Anyways, I took this verbatim into my PR. The extended suite of tests and refactoring is probably a good idea anyways, if we now are going forward with the path normalization.
There was a problem hiding this comment.
USERPROFILE is a variable that is only defined on windows, and our home_dir only uses it on windows.
On windows, it will contain \, not / .
The difference between the regression in your PR, and the change here, is that here, the change in the test, makes the test closer to the environment, that it tested.
In your PR, the change was done just to make the CI pass with the changes in the PR.
| assert os.join_path('/', 'test') == '/test' | ||
| assert os.join_path('foo/bar', './file.txt') == 'foo/bar/file.txt' | ||
| assert os.join_path('/opt/v', './x') == '/opt/v/x' | ||
| assert os.join_path('./a', './b') == './a/b' |
There was a problem hiding this comment.
why not a/b here? I see no reason to keep the initial ./ prefix.
There was a problem hiding this comment.
Good point. It will be simpler without it.
There was a problem hiding this comment.
It will have to be done in a separate focused PR, since it will break asserts like this one cmd/tools/vcomplete_test.v:229:
> assert line == sorted_expected[i]
Left value: sub0/
Right value: ./sub0/
, and is not directly related to the join_path('/', 'x') problem in the issue.
There was a problem hiding this comment.
It is implemented in https://github.com/spytheman/v/tree/cleanup_dot_slash_prefix_at_start_of_output_in_join_path for now.
| assert os.join_path('foo\\bar', '.\\file.txt') == r'foo\bar\file.txt' | ||
| assert os.join_path('/opt/v', './x') == r'\opt\v\x' | ||
| } $else { | ||
| assert os.join_path('./b', '') == './b' |
|
Obsoleted by #21494 . |
Fix #20231 .