Skip to content
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

Merged
merged 1 commit into from
Aug 5, 2018

Conversation

timotheecour
Copy link
Member

No description provided.

## 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] == '\\'):
Copy link
Contributor

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

Copy link
Member Author

@timotheecour timotheecour Jul 19, 2018

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)

Copy link
Contributor

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.

Copy link
Member Author

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.

@Araq
Copy link
Member

Araq commented Jul 20, 2018

What bug does this fix?

@timotheecour
Copy link
Member Author

it makes expandTilde "~" work: previously, it returned "~" ; after PR, returns "/home/timothee" ; ie, same as unix, python, D
it also refactors to use DirSep, AltSep and generates runnableExamples which are improvements on their own
(as title says)

@data-man
Copy link
Contributor

Windows specific, from Naming Files, Paths, and Namespaces:

Use any character in the current code page for a name, including Unicode characters and characters in the extended character set (128–255), except for the following:

The following reserved characters:

< (less than)
> (greater than)
: (colon)
" (double quote)
/ (forward slash)
\ (backslash)
| (vertical bar or pipe)
? (question mark)
* (asterisk)

So dir's name can equal/starts to/from ~

@timotheecour
Copy link
Member Author

same on posix:

 echo foo >> '~' && ls
total 4.0K
-rw-r--r-- 1 timothee 8 Jul 20 20:56 '~'

~ is legal character, but expandTilde will interpret it as meaning home
(that behavior was pre-existing to this PR, just in a slightly buggy form)

@dom96 dom96 merged commit 6fffadb into nim-lang:devel Aug 5, 2018
@timotheecour timotheecour deleted the pr_expandTilde branch August 5, 2018 20:52
@timotheecour
Copy link
Member Author

timotheecour commented Aug 6, 2018

@dom96 thanks for merging.
btw: for next time, IMO the committer should use 'Rebase and merge' instead of merge to avoid generating non-linear history, and ping the PR author in case there are rebase conflicts at time of merge, whenever possible
rationale: https://github.com/nim-lang/Nim/pull/8271/files#diff-cc4aac3e9be04e0413c9520f223b493cR12
see also @skilchen's remark about not being able to use git bisect after merge: #8432 (comment)

(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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants