Skip to content

Conversation

genotrance
Copy link
Contributor

@genotrance genotrance commented Mar 30, 2018

Added the following procs to nimscript:-

I was going to add some test cases into tests/newconfig/tfoo.nims but realized the nims extension is not covered by testament.

@data-man
Copy link
Contributor

Maybe to rename a functions?
cpDir() -> copyDir()
mvDir() -> moveDir()
pwd() -> getCurrentDir()
where() -> findExe()

For compatibility with stdlib.
And pwd() for Windows-users it's not clear. And where() for *x-users. :)

@genotrance
Copy link
Contributor Author

genotrance commented Mar 31, 2018

@Araq preferred different names which is why I did it this way.

https://irclogs.nim-lang.org/29-03-2018.html#23:33:13

## Moves the dir `from` to `to`.
log "mvDir: " & `from` & ", " & to:
moveDir `from`, to
checkOsError()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to your PR, but why is this checkOsError needed? Won't movedDir etc. raise OSError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exceptions are caught in template cbos() and errorMsg is set there for checkOsError() to check and re-raise it.

@dom96
Copy link
Contributor

dom96 commented Apr 1, 2018

@Araq preferred different names which is why I did it this way.

It has just occurred to me why perhaps following Rust's convention might have made sense, exclamation mark at the end of every compile-time proc. Of course this would have had to be done before Rust was even a thing :)

@Araq
Copy link
Member

Araq commented Apr 6, 2018

  1. Rust uses the exclamation mark for macros.
  2. Exclamation marks are ugly and patronizing. Nim is not patronizing.

@Araq
Copy link
Member

Araq commented Apr 6, 2018

Sorry this took so long to review. The names are wrong. NimScript names differ from the stdlib names iff they do different things. rmDir is not an alias for removeDir it logs its action and supports ScriptMode.Whatif. But your where and pwd are just aliases, these should be named findExe and getCurrentDir. Btw getCurrentDir is already supported in nimscript.nim.

@genotrance
Copy link
Contributor Author

I noticed getCurrentDir() but it isn't exported. I also think it is fair to have unique names in nimscript so that once import os does work in nimscript, it won't be a proc name conflict. Especially for backward compatibility, we won't be able to remove getCurrentDir() from nimscript as existing scripts will break without "import os".

I can change where() and pwd() back to their original names if there's a way to avoid the conflict in the future without requiring any script changes.

@Araq
Copy link
Member

Araq commented Apr 6, 2018

Well currently you cannot import os in NimScript and once you can you need to disambiguate via import os except getCurrentDir or similar. Not ideal but better than changing the reasoning behind these names.

@genotrance
Copy link
Contributor Author

Sounds fair since there's also fileExists(), paramCount() and other such procs which share names so this isn't anything new.

I'll change to getCurrentDir() and findExe() and push.

@genotrance
Copy link
Contributor Author

Added to changelog under "Library additions".

@Araq Araq merged commit f6c8f97 into nim-lang:devel Apr 10, 2018
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