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

getCurrentDir should be in ospaths, not os #9523

Closed
timotheecour opened this issue Oct 27, 2018 · 13 comments
Closed

getCurrentDir should be in ospaths, not os #9523

timotheecour opened this issue Oct 27, 2018 · 13 comments
Labels
OS (stdlib) Related to OS modules in standard library

Comments

@timotheecour
Copy link
Member

timotheecour commented Oct 27, 2018

/cc @demotomohiro @Araq @dom96

a number of procs are in os instead of ospaths just because getCurrentDir is defined in os instead of ospaths, eg:

  • proc absolutePath*(path: string, root = getCurrentDir()) (because of getCurrentDir)
  • normalizedPath (because it depends on isAbsolute)
  • it also prevents having an overload of relativePath with 1 param as found in lots of languages (D, python, boost), as it would then force relativePath to be in os instead of ospaths (see Add relativePath proc and test #8166 (comment) for that point)

Furthermore, we already use getEnv to access environment variables in ospaths, so using the low-level c procs getcwd/getCurrentDirectoryW wouldn't be an issue IMO

proposal

move getCurrentDir, absolutePath, normalizedPath to ospaths
this would have zero impact on existing code

@dom96
Copy link
Contributor

dom96 commented Oct 27, 2018

ospaths should be merged back into os.

@krux02
Copy link
Contributor

krux02 commented Oct 27, 2018

Just make sure to always import both, os and ospaths. I am closing this, because it is too much opinion based.

@krux02 krux02 closed this as completed Oct 27, 2018
@timotheecour
Copy link
Member Author

timotheecour commented Oct 29, 2018

@krux02 do you mind re-opening this? too much opinion based is also opinion based after all

There was not enough time for feedback. Just make sure to always import both, os and ospaths is akin to saying ospaths should be merged in os, and I'm not sure we wanna go there either (at least that's a separate discussion from this one)

ospaths is meant for pure path manipulation, but for pragmatic reasons including currentDir (and as is already the case, environment variable access) makes sense so that these argulably path manipulation procs can also be included there:

  • absolutePath, normalizePath, relativePath

and btw I'm happy to write the PR myself once this is accepted, and expect it'll lead to zero breakage

@timotheecour timotheecour changed the title getCurrentDir should be in ospaths, not os [TODO] getCurrentDir should be in ospaths, not os Oct 29, 2018
@krux02
Copy link
Contributor

krux02 commented Oct 29, 2018

If you want to discuss this, discuss it in the chat. The issue tracker is not for discussions. But if you really really want this to be open, I make you an offer. You alone have 200 open issues in Nim right now. They can't be all that important, if you look through them and remove three of the most unimportant ones, then I can reopen this issue.

@Araq
Copy link
Member

Araq commented Oct 29, 2018

They can't be all that important, if you look through them and remove three of the most unimportant ones, then I can reopen this issue.

Come on.... That's just ... weird politics.

@Araq Araq reopened this Oct 29, 2018
@krux02 krux02 added OS (stdlib) Related to OS modules in standard library and removed RFC labels Oct 29, 2018
@krux02 krux02 changed the title [TODO] getCurrentDir should be in ospaths, not os getCurrentDir should be in ospaths, not os Oct 29, 2018
@timotheecour
Copy link
Member Author

timotheecour commented Oct 29, 2018

If you want to discuss this, discuss it in the chat

not everyone follows chat at all times, and not everyone is on same timezone; chat is great (and more efficient) when you want to discuss something with specific person(s) that is logged in, or when you want help, not so much when you're discussing something that other people may care about; otherwise it leads to "behind closed doors" decisions (yes, irc/gitter is searchable but anything older than 1 day is hard to search unless you know when something was discussed); at least forum is better than IRC for that.

@Araq
Copy link
Member

Araq commented Oct 30, 2018

It's time we merge ospaths back into os.

@timotheecour
Copy link
Member Author

@Araq @dom96 back in the days, 178275f split ospaths from os with commit msg: split os into os and ospaths parts; ospaths is available for NimScript; better NimScript support
what would be implication for NimScript if ospaths were merged back into os?

independently of that, I prefer keeping ospaths separate from os (and already sent out a PR for addressing this issue), it just seems cleaner to separate out path manipulation procs (+ getCurrentDir for pragmatism) from rest of os for sake of modularity
(one additional argument could be we may want to consider a future ospaths2 that replaces ospaths based on typesafe paths)

@Araq
Copy link
Member

Araq commented Oct 30, 2018

what would be implication for NimScript if ospaths were merged back into os?

That only some procs in os.nim are available for NimScript. They need to be documented.

independently of that, I prefer keeping ospaths separate from os (and already sent out a PR for addressing this issue), it just seems cleaner to separate out path manipulation procs (+ getCurrentDir for pragmatism)

But "+ getCurrentDir for pragmatism" means the split doesn't work well. Anyhow, ospaths is not just OS independent string processing anymore, so it's all messy, having a single os.nim feels much less messy.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 30, 2018

well the proper fix here would be to allow cyclic dependencies (with care taken so that import os doesn't import the world, which can be enforced via tooling that computes module import graph and some checks to make sure we don't accidentally add unwanted dependencies)

a single import os also means more potential for symbol clashes as os 's scope is much larger than ospaths

@Araq
Copy link
Member

Araq commented Oct 30, 2018

That's not the "proper fix" that's a terrible design. And symbol clashes are a thing for import os except ... or from os import nil.

@timotheecour
Copy link
Member Author

timotheecour commented Nov 6, 2018

That's not the "proper fix" that's a terrible design

this should be discussed in a separate place dedicated for cyclic import, I don't want to conflate issues.

And symbol clashes are a thing for import os except ... or from os import nil.

I regret raising that point; yes, these work for symbol clashes.

But my point about modularization still stands;
/cc @krux02
that's not an opinion but a fact: in pretty much all languages I've used you have an ospaths equivalent that isn't bundled with the rest of os, for reasons I already mentioned: os is much larger in scope and ospaths scope is relatively well defined:

I don't mind Nim doing things differently from every other language when there's an advantage to it, but this isn't the case here. I don't see any cons to the PR I sent out.

@Araq
Copy link
Member

Araq commented Nov 6, 2018

On the contrary, moving existing stuff around pointlessly has no advantage.

@Araq Araq closed this as completed Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS (stdlib) Related to OS modules in standard library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants