fix issue #8268 (joinPaths); add lots of tests and make use of use runnableExamples #8351

timotheecour · 2018-07-18T00:11:53Z

/cc @dom96

fixes https://github.com/nim-lang/Nim/issues/8268 by adding an optional absOverrides to joinPath (by default, same behavior as before so no breaking change)
correctly handles boundary bw head and tail so that there's a unique dirsep between
adds normalizePathEnd (also useful in other places, can be refactored later)
adds lots of tests (and fixes other things that were broken with joinPaths)
add countWhile and rootPrefixLength (simplifies and fixes a lot of code)

eg motivation

joinPath(foo, bar) is often a bug when bar is absolute, for eg:
nimterop/nimterop@a8bb2dc#r31391178

don't joinPath if path.isAbsolute

Varriount · 2018-07-18T08:36:23Z

Why is normalizePathEnd public? Even if required by other procedures in the os module, it seems somewhat niche.

haltcase · 2018-07-18T18:52:18Z

lib/pure/ospaths.nim

+      doAssert joinPath("usr///", "//lib") == "usr/lib" ## `//` gets compressed
+      doAssert joinPath("//", "lib") == "/lib" ## ditto
+    when defined(Windows):
+      doAssert joinPath("""C:\foo""", """D:\bar""") == """C:\foo\bar"""


Should be able to swap the triple quotes out for raw strings:

doAssert joinPath(r"C:\foo", r"D:\bar") == r"C:\foo\bar"

done, thx for the tip!

haltcase · 2018-07-18T18:53:01Z

lib/pure/ospaths.nim

-  ##   assert joinPath("usr/", "/lib") == "usr/lib"
+  ## If ``tail`` is absolute or ``head`` is empty, returns ``tail``. If
+  ## ``tail`` is empty returns ``head``. Else, returns the concatenation with
+  ## normalized spearator between ``head`` and ``tail``.


typo: spearator -> separator

timotheecour · 2018-07-18T22:03:07Z

Why is normalizePathEnd public? Even if required by other procedures in the os module, it seems somewhat niche.

I think it's useful, I would want to use it in my code

timotheecour · 2018-07-19T01:31:52Z

PTAL, now green

Varriount · 2018-07-19T10:28:13Z

@dom96 What are your thoughts? Aside from my thoughts on normalizePathEnd, this PR looks ready.

dom96 · 2018-07-19T12:25:13Z

lib/pure/ospaths.nim

@@ -157,64 +158,173 @@ const
    ## The character which separates the base filename from the extension;
    ## for example, the '.' in ``os.nim``.

+# TODO: MOVE to strutils, sequtils, or algorithms
+proc countWhile*(str: string, pred : proc(a:char):bool, prefix = true): int =


You can use parseutils.parseWhile for this, or just str.len-str.replace("/", "").len

If you want more complicated counting then just use a for loop...

parseWhile is not as flexible (eg I need arbitrary lambda; and I need to go from both ends).

str.len-str.replace("/", "").len won't work with my other usages of countWhile.

If you want more complicated counting then just use a for loop...

countWhile leads to simpler code

dom96 · 2018-07-19T12:27:50Z

lib/pure/ospaths.nim

+  return lenS
+
+# TODO: once slices are supported, define instead `rootPrefix`
+proc rootPrefixLength*(path: string) : int =


Once slices are supported? Just return the prefix and call len on it.

rootPrefixLength avoids allocating. If/when we have slices, this wouldn't be a problem; until then it is.

dom96 · 2018-07-19T12:44:56Z

lib/pure/ospaths.nim

+      doAssert r"C:".rootPrefixLength == 2
+      doAssert r"C:\foo".rootPrefixLength == 3
+      doAssert r"//foo".rootPrefixLength == 2
+      doAssert r"C:\\foo".rootPrefixLength == 4


As far as I'm concerned this is abusing runnableExamples as exhaustive testing again. Examples should be one or two (one per platform). Not a long list of edge cases.

fixed; moved out from runnableExamples

fixed; moved out of runnableExamples

dom96 · 2018-07-19T12:47:27Z

lib/pure/ospaths.nim

+    if path[0] == ':':
+      result = 0
+    else:
+      result = countWhile(path, a => a != ':')


Is macos pre-mac-os-x only? If it covers latest macOS then this looks wrong to me.

IIRC macos is the legacy classic Mac, macosx is, well, Mac OS X. This is kind of confusing though because since 10.12 Sierra they rebranded it as macOS.

yeah, this is far too confusing.

what is too confusing? the code in PR or macos? I already suggested removing support for macos https://forum.nim-lang.org/t/4039 "should we keep supporting macos? (as opposed to macosx)"

dom96 · 2018-07-19T12:48:13Z

lib/pure/ospaths.nim

+      doAssert "/".isAbsolute
+      doAssert(not "a/".isAbsolute)
+    when defined(Windows):
+      doAssert "C:\\foo".isAbsolute


Too many examples here too.

dom96 · 2018-07-19T12:51:09Z

lib/pure/ospaths.nim

-  ##   assert joinPath("", "lib") == "lib"
-  ##   assert joinPath("", "/lib") == "/lib"
-  ##   assert joinPath("usr/", "/lib") == "usr/lib"
+  ## If ``tail`` is absolute and ``absOverrides`` is true, or ``head`` is empty,


I'm not really sure about the use case for this.

rationale is given in #8268 : most languages use the behavior absOverrides=true whereas before this PR nim was using the equivalent of absOverrides=false; so with this PR, 0 code is broken but user has option of using absOverrides=true

dom96 · 2018-07-19T12:54:08Z

I also think that normalizePathEnd is niche and shouldn't be there. I'm unsure about what it's purpose is in joinPath.

dom96 · 2018-07-19T12:56:27Z

All in all there is too much going on here. I would prefer if you just created a simple PR that fixes the underlying issue instead of introducing two new procedures into the mix, which as far as I'm concerned are not needed. I'm pretty sure for most purposes normalizePathEnd can just be if path[^1] == '/': path[0 .. ^2]

timotheecour · 2018-07-20T00:20:03Z

/cc @Araq @dom96

I'm pretty sure for most purposes normalizePathEnd can just be if path[^1] == '/': path[0 .. ^2]

doesn't work with path = "foo//" and other edge cases. The way I wrote it via normalizePathEnd is robust and can be reused instead of being wrong some of the time (and doing it by hand each time); I found a lot of bugs in ospaths module, common root cause is lack of encapsulation/helper functions like these.

I also think that normalizePathEnd is niche and shouldn't be there. I'm unsure about what it's purpose is in joinPath.

I have other upcoming PR's that fix pre-existing bugs with ospaths that rely on normalizePathEnd, countWhile, rootPrefixLength. Having these helpers makes code simpler, more reusable, less buggy.

I'm willing to address the comments that make sense to me:

moving some examples to tos.nim instead of runnableExamples
moving countWhile to strutils.nim
any other comment that makes sense to me

but I'm not ok with removing these helpers. Let me know if you want me to go ahead with this plan otherwise I'll just apply these fixes in a private library and someone else can fix these issues in ospaths (there are many, see a list #8177 + others I haven't reported).

Araq · 2018-07-20T07:47:38Z

but I'm not ok with removing these helpers. Let me know if you want me to go ahead with this plan otherwise I'll just apply these fixes in a private library and someone else can fix these issues in ospaths (there are many, see a list #8177 + others I haven't reported).

I'm not ok that you have to rewrite large parts of mostly working code with your own ideas of how things should be done in Nim/D and leaving a track record of well meaning, but time-consuming RFCs/proposals as a side effect that take away from our manpower. I really appreciate your help, but if in the end your help makes us all more unproductive than we were before, I have to decline it.

Let's assume ospaths.nim has 20 bugs (believe it or not, many people use it effectively, today!). Every fix is maybe 5 lines of code, every fix can be a tiny PR that keeps the CIs green. That means the core dev get to review 20 little PRs that can be accepted in maybe one hour. These PRs also probably wouldn't add new dependencies between the stdlib modules, btw.

Your way of working: Write 20 RFCs instead, half of them about perceived more general language deficiencies. And here is the bad part of it: More/different language features just move problems around most of the time. For example, I don't want to use D's superior const system, I know from experience it's not worth it, given its complexity, it doesn't find enough real bugs. (Same holds for Nim's tags effect system.) I don't want to use D's superior "copy-free" slicing system either, it introduces problematic aliasing.

…") on windows

timotheecour · 2018-08-27T02:52:21Z

/cc @Araq PTAL

no new dependency added (removed the import sugar I had added)
no new public API (made countWhile private; and incorporated all comments from [TODO] strutils.countWhile: counts while a predicate is true, starting from either end #8673 except s/proc/set/ ; it's private so shd be fine ; I would've preferred if [TODO] strutils.countWhile: counts while a predicate is true, starting from either end #8673 hadn't been closed but, moving on...)
runnableExamples kept to reasonable size
moved tests to tests/stdlib/tospaths.nim except ones that refer to private symbols
some code had to be moved in this PR (eg normalizePathEnd which was already committed from a previous PR) ; you can verify for yourself that the move doesn't introduce changes using git diff --color-moved ; I tried using {.reorder:on.} but it exposed bugs in reorder:on implementation (will send a bug for that) ; also tried using function declaration but also led to other issues; so I just moved up some code.

PS: I'm not gonna attempt defending my POV and instead will focus here on moving forward with this PR

Araq · 2019-01-10T17:07:17Z

Needs to be redone, os.nim changed significantly.

timotheecour force-pushed the joinPath_abs branch 3 times, most recently from cbc9e4d to fc1ffb0 Compare July 18, 2018 00:17

timotheecour force-pushed the joinPath_abs branch from faf723e to ec3ea7b Compare July 18, 2018 18:16

haltcase reviewed Jul 18, 2018

View reviewed changes

timotheecour force-pushed the joinPath_abs branch 2 times, most recently from 27aca68 to 16eebf9 Compare July 18, 2018 21:58

timotheecour force-pushed the joinPath_abs branch from 16eebf9 to f79a016 Compare July 19, 2018 00:39

timotheecour mentioned this pull request Jul 19, 2018

8268 fix #8361

Closed

dom96 requested changes Jul 19, 2018

View reviewed changes

timotheecour mentioned this pull request Aug 18, 2018

[ospaths] fix #8491: XDG_CONFIG_HOME was being ignored due to #8681 + private normalizePathEnd #8680

Merged

timotheecour force-pushed the joinPath_abs branch 2 times, most recently from c07b318 to 6e3be1e Compare August 27, 2018 00:11

timotheecour added 3 commits August 26, 2018 17:17

simplify compiler/reorder:joinPath

22e1ca6

add runnableExamples to ospaths.joinPath

94bab92

fix issue #8268 (joinPaths); add lots of tests

fd4e063

timotheecour force-pushed the joinPath_abs branch from 6e3be1e to 924c963 Compare August 27, 2018 00:17

new procs countWhile, rootPrefixLength; fix joinPath("foo", "\bar…

562b88a

…") on windows

timotheecour force-pushed the joinPath_abs branch 3 times, most recently from 138c550 to 73c2851 Compare August 27, 2018 00:51

fixup

ddc6850

timotheecour force-pushed the joinPath_abs branch from 73c2851 to ddc6850 Compare August 27, 2018 02:23

timotheecour changed the title ~~fix issue #8268 (joinPaths);add normalizePathEnd; add lots of tests~~ fix issue #8268 (joinPaths); add lots of tests and make use of use runnableExamples Aug 27, 2018

timotheecour added the OS (stdlib) Related to OS modules in standard library label Oct 25, 2018

timotheecour mentioned this pull request Dec 17, 2018

[regression] [os] joinPath("usr/", "/lib")="usr/lib" but joinPath(".", "/lib")="/lib" #10025

Closed

Araq closed this Jan 10, 2019

timotheecour added the TODO items remaining label Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue #8268 (joinPaths); add lots of tests and make use of use runnableExamples #8351

fix issue #8268 (joinPaths); add lots of tests and make use of use runnableExamples #8351

timotheecour commented Jul 18, 2018 •

edited

Loading

Varriount commented Jul 18, 2018

haltcase Jul 18, 2018

timotheecour Jul 18, 2018

haltcase Jul 18, 2018

timotheecour Jul 18, 2018

timotheecour commented Jul 18, 2018

timotheecour commented Jul 19, 2018

Varriount commented Jul 19, 2018

dom96 Jul 19, 2018

timotheecour Jul 20, 2018

dom96 Jul 19, 2018

timotheecour Jul 20, 2018

dom96 Jul 19, 2018

timotheecour Aug 27, 2018

timotheecour Aug 27, 2018

dom96 Jul 19, 2018

haltcase Jul 19, 2018 •

edited

Loading

dom96 Jul 19, 2018

timotheecour Jul 19, 2018

dom96 Jul 19, 2018

timotheecour Aug 27, 2018

dom96 Jul 19, 2018

timotheecour Aug 27, 2018

dom96 commented Jul 19, 2018

dom96 commented Jul 19, 2018 •

edited

Loading

timotheecour commented Jul 20, 2018 •

edited

Loading

Araq commented Jul 20, 2018

timotheecour commented Aug 27, 2018 •

edited

Loading

Araq commented Jan 10, 2019

fix issue #8268 (joinPaths); add lots of tests and make use of use runnableExamples #8351

fix issue #8268 (joinPaths); add lots of tests and make use of use runnableExamples #8351

Conversation

timotheecour commented Jul 18, 2018 • edited Loading

eg motivation

Varriount commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Jul 18, 2018

timotheecour commented Jul 19, 2018

Varriount commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haltcase Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dom96 commented Jul 19, 2018

dom96 commented Jul 19, 2018 • edited Loading

timotheecour commented Jul 20, 2018 • edited Loading

Araq commented Jul 20, 2018

timotheecour commented Aug 27, 2018 • edited Loading

Araq commented Jan 10, 2019

timotheecour commented Jul 18, 2018 •

edited

Loading

haltcase Jul 19, 2018 •

edited

Loading

dom96 commented Jul 19, 2018 •

edited

Loading

timotheecour commented Jul 20, 2018 •

edited

Loading

timotheecour commented Aug 27, 2018 •

edited

Loading