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

isValidFilename seems incorrect or misleading #17922

Open
timotheecour opened this issue May 2, 2021 · 3 comments
Open

isValidFilename seems incorrect or misleading #17922

timotheecour opened this issue May 2, 2021 · 3 comments
Assignees

Comments

@timotheecour
Copy link
Member

timotheecour commented May 2, 2021

Example 1

when true:
  import std/os
  let name = " foo.tmp"
  writeFile(name, "baz") # works
  echo isValidFilename(name) # false

Current Output

false

Expected Output

true, since writeFile works

Example 2

the example should be converted to runnableExamples

Example 3

isValidFilename("") crashes

Possible Solution

  • add an optional param to isValidFilename to specify whether the filename is actually valid or (subjective) follows recommended guidelines (eg no space)
  • or fix isValidFilename implementation
  • or at very least clarify semantics in doc comments

Additional Information

1.5.1 ee6d561
on osx

i was exploring whether to use this to address #17914 (comment) but turns out it's useless for this purpose (and also doesn't check presence of DirSep etc)

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented May 2, 2021

It says func only checks filenames very explicit, separators are not part of the "filename" by definition.

It is for when you need that a filename works on all OS without problems,
some filenames works on a given OS, and crash in another OS,
example you can create a file named CON on Linux, but it crashes your Windows.

Who wants filenames with trailing whitespaces anyway?.

Therefore "writeFile works" is not an absolute way to determine anything.
🤷

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented May 2, 2021

Maybe a new isValidPath() ?,
should it check whole paths, including separators, if the disk actually exists (C:/, Z:/, /something/, samba:/, etc),
if the disk is mounted, if the disk is an unix NFS is it network reachable, if the disk is an SSHFS do you have the keys,
may get more complicated, and not as cheap as the filename check, etc.

@timotheecour timotheecour self-assigned this May 2, 2021
@haoyu234
Copy link
Contributor

haoyu234 commented May 7, 2021

why not isValidFileName, just like randomPathName #17898

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 a pull request may close this issue.

3 participants