fix #10630: nim doc uses wrong definition for DirSep on windows, causing all sorts of bugs

[timotheecour]

• fix [TODO] nim doc uses wrong definition for DirSep on windows, causing all sorts of bugs #10630: nim doc uses wrong definition for DirSep on windows, causing all sorts of bugs

• can this be [backport] even though lib/pure/includes/osseps.nim was introduced in 9cc4a57 ?

[krux02]

This diff makes it really hard to see what actually has been changed.

[narimiran]

nim doc uses wrong definition for DirSep on windows, causing all sorts of bug

...and from what I can tell from briefly scrolling through the changes, you used this opportunity to change much more than that, introducing ...Impl everywhere.

Btw, sorry to be blunt, but I will not consider/review your PRs until the count of your open issues is above 150. Close some least important ones (which affect the least amount of Nim users), this will show some good will from your side, and then I'll change my mind.

[timotheecour]

This diff makes it really hard to see what actually has been changed.

see the paragraph I added here: https://nim-lang.github.io/Nim/contributing.html#the-git-stuff-code-reviews

When reviewing large diffs that may involve code moving around, github's interface doesn't help much as it doesn't highlight moves. Instead you can use something like this, see visual results here:git fetch origin pull/10431/head && git checkout FETCH_HEAD
git show --color-moved-ws=allow-indentation-change --color-moved=blocks HEAD^
In addition, you can view github-like diffs locally to identify what was changed within a code block using diff-highlight or diff-so-fancy, eg:# put this in ~/.gitconfig:
[core]
  pager = "diff-so-fancy | less -R" # or: use: `diff-highlight`

...and from what I can tell from briefly scrolling through the changes, you used this opportunity to change much more than that, introducing ...Impl everywhere.

I applied the minimum change I could think of that fixed the bug; could you please suggest an alternative that doesn't introducing Impl ? The point is to have doc comments in 1 place, but with machine specific implementations for each symbol.

Btw, sorry to be blunt, but I will not consider/review your PRs until the count of your open issues is above 150. Close some least important ones (which affect the least amount of Nim users), this will show some good will from your side, and then I'll change my mind.

I really don't think this is a good idea (eg see #10257 (comment) for my position on this point) but I will go ahead and close some issues I created; just don't blame me if people complain.

[timotheecour]

Btw, sorry to be blunt, but I will not consider/review your PRs until the count of your open issues is above 150. Close some least important ones (which affect the least amount of Nim users), this will show some good will from your side, and then I'll change my mind.

@narimiran i closed some issues and tagged them as "Won't fix" but I just lost ability to tag issues in the past 10 minutes, so I can't tag the issues I'm closing with "Won't fix" anymore. This is not helping at all.

[narimiran]

I can't tag the issues I'm closing with "Won't fix" anymore.

But you can still close them ;)

I'll tag them later, don't worry, just continue with the cleanup.

[timotheecour]

that's really counter-productive. I can just avoid creating new labels from now on, no need to remove my ability to tag issues altogether.

[krux02]

that's really counter-productive.

I am sorry to be blunt here, but dealing with you is counter productive. Your issues are convoluted, ignore the issue templates, and generally disrespect our time to read them.

[timotheecour]

@narimiran

Btw, sorry to be blunt, but I will not consider/review your PRs until the count of your open issues is above 150. Close some least important ones (which affect the least amount of Nim users), this will show some good will from your side, and then I'll change my mind.

done, 148. I went through all my remaining open issues and they're all valid, IMO. EDIT: 144

[narimiran]

done, 148.

Great, now I went through this PR and here are my questions:

1. In the title you claim "causing all sorts of bugs" -> Can you back that up with some data? What sorts of bugs? Where?

2. What exactly is wrong? The current documentation for DirSep says:

    DirSep* = '/'
      ## The character used by the operating system to separate pathname
      ## components, for example: `'/'` for POSIX, `':'` for the classic
      ## Macintosh, and `'\\'` on Windows.

Yeah, it shows '/' but it clearly explains it is '\\' on Windows.

I briefly skimmed through the PR, and it seems to me (correct me if I'm wrong) like all these changes are there just one line in the documentation will be changed to DirSep* = '\\'.

Having that said, this is a big no from me.

[GULPF]

Yeah, it shows '/' but it clearly explains it is '\\' on Windows.

The point is that when running nim doc it wont be '\\' on Windows, it will always be '/'. Since nim doc performs semcheck, and semcheck can execute arbitrary code, this could definitely cause nim doc to fail when it shouldn't. It's hard to imagine a real world situation where this would be a problem for DirSep, but this PR is still an improvement so it shouldn't be closed.

[Araq]

@GULPF The better way would have been const dirSep = selectSep('/', '\\') ## doc comment here though.