-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
ospaths.expandTilde: handle ~ correctly; refactor to use DirSep, AltSep #8364
Conversation
## let configFile = expandTilde("~" / "appname.cfg") | ||
## echo configFile | ||
## # --> C:\Users\amber\appname.cfg | ||
if len(path) > 1 and path[0] == '~' and (path[1] == '/' or path[1] == '\\'): |
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.
Why not just:
if len(path) >= 1 and path[0] == '~' and (path.len == 1 or path[1] in {DirSep, AltSep}):
or if you like separate if branches then I would prefer this way:
if path.len >= 1 and path[0] == '~':
return getHomeDir() / path.substr(2)
else:
return 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.
your suggestion returns wrong results:
echo extractFilename("~") # returns /home/timothee/ instead of /home/timothee
# (expandTilde shouldn't add trailing slash, and should be like python os.path.expanduser or D's expandTilde)
echo extractFilename("~bob") # returns /home/timothee/ob instead of `~bob`
# (unmodified, see TODO I left for future PR)
# or the more correct /home/bob (future PR)
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.
Why is a trailing slash such a big issue?
In any case, I'm sure you can adjust my code to fix these. I wrote these quickly, the main point is the style of code. The ordering of your if branches in this PR is a little confusing.
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.
Why is a trailing slash such a big issue?
because of things like #8341
currently extractFilename("/home/timothee/")
!= extractFilename("/home/timothee")
(that particular case may hopefully be fixed, but there could be other cases where adding a trailing slash changes things)
I can't think of a simpler way to have same semantics as what I wrote, but if you have a concrete suggestion, happy to take it. Note that the last case ~bob and ~bob/ is a separate case, to be handled in a future PR.
What bug does this fix? |
it makes |
Windows specific, from Naming Files, Paths, and Namespaces:
So dir's name can equal/starts to/from |
same on posix: echo foo >> '~' && ls
total 4.0K
-rw-r--r-- 1 timothee 8 Jul 20 20:56 '~'
|
@dom96 thanks for merging. (IIRC last time I updated this PR it was either rebased against latest devel or rebaseable with no conflict, but time has passed since then so idk if it was still rebaseable with no conflict when you did the merge. Happy to resolve any rebase conflicts if I knew you were ok with merging now) |
No description provided.