fix #8225 os.isHidden was buggy on posix #8315

timotheecour · 2018-07-14T07:45:29Z

/cc @dom96
fixes #8225
fixed bugs on posix:

isHidden shouldn't care whether file exists or not (purely string operation)
now works for dirs, not just files so that isHidden(".git") now returns true
there was also a bug where .ab.txt was considered not hidden (even if it existed!) because of the weird condition fileName[3] != '.'
EDIT: use the new lastPathPart proc so that isHidden(".git/") now returns true as well

Varriount · 2018-07-14T10:41:53Z

With regards to the "this should purely be a string operation", this makes the behavior of the function subtly different across Linux and Windows. It means that on Windows, the function can only be called on existing files, while on Linux it can be called on all possible file names.

timotheecour · 2018-07-14T12:11:37Z

well I had suggested #8225 (comment) which would've mostly avoided that issue via a cross platform isDotPath (called by os.isHidden on posix):

ospaths.isDotPath => checks only for paths, works cross platform
os.isHidden => calls ospaths.isDotPath on posix, calls windows specific on windows (ie, same as my top level recommendation)

but @dom96 suggested not to:

Don't add any new procs. Just implement your proposal.

That just reflects OS differences; can't completely hide differences here

dom96 · 2018-07-14T12:14:35Z

lib/pure/os.nim

+  ## NOTE: on OSX, in addition to the "dotfile" behaviour, existing paths with the
+  ## "Invisible" attribute are hidden in Finder, although not in ls
+  ## NOTE: the behavior of certain corner cases (see examples) ".foo/" is
+  ## unspecified, subject to change (can be avoided using `normalizePath`)


When writing docs, please check what they look like with nim doc. Currently this is all going to be one huge paragraph, and therefore hard to read. Add empty ## to separate this and use bold instead of ALLCAPS to emphasise.

on that note is there a canonical "perfect" reference function we can use as a template? (that was one of my goals with #8271 btw)

Don't have a "perfect" function to link to, but here is how these should be structured:

proc foo() = ## Performs the foo operation. ## ## The foo operation is a very important operation ## in computer science. It solves all our problems. ## ## **Warning:** Calling ``foo`` when the ``blarg`` ## was activated previously may cause the world to ## end.

I've seen both these:

`foo` vs ``foo``

and both work (and foo is simpler and more familiar for ppl familiar w markdown); ok to use single ticks ?

Nope, they aren't the same in RST.

indeeed, for posterity:

`foo` vs ``foo`` vs foo

it's definitely not used consistently though. should that be handled by nimfix? nimpretty?

Don't have a "perfect" function to link to, but here is how these should be structured:

add it to contributing.md #8271 in b370902

dom96 · 2018-07-14T12:16:05Z

lib/pure/os.nim

+      doAssert(not "".isHidden)
+      # TODO: decide on these:
+      # doAssert ".foo/".isHidden # un-specified
+      # doAssert ".foo/.".isHidden # un-specified


Don't abuse runnableExamples for a test suite. The docs should show one or two examples, not an exhaustive list of checks.

done, kept the minimum sufficient set to clarify semantics

timotheecour · 2018-07-14T20:49:32Z

@dom96 PTAL

…(adapted from nim-lang#8315 (comment))

dom96 · 2018-07-15T11:18:30Z

lib/pure/os.nim

+  ## On posix: a hidden path starts with ``.`` (see examples).
+  ##
+  ## On OSX, in addition to the "dotfile" behaviour, existing paths with the
+  ## "Invisible" attribute are hidden in Finder, although not in ``ls``.


Why are you discussing Finder's behaviour here? This is confusing because it implies that this function follows the Finder behaviour but I don't think that's the case.

removed that comment (which was just to say there's a difference bw Finder's hidden vs ls's hidden, even though this function deals with ls's hidden)

dom96 · 2018-07-15T11:19:14Z

lib/pure/os.nim

+    result = len(fileName) >= 2 and fileName[0] == '.' and fileName != ".."
+
+when isMainModule:
+  doAssert ".foo.ext".isHidden


Put these into tos please: https://github.com/nim-lang/Nim/blob/devel/tests/stdlib/tos.nim

done (also moved absolutePath tests there)

lib/pure/os.nim

Varriount · 2018-07-21T02:40:57Z

@timotheecour This is still failing tests.

haltcase

Seems to just be an indentation error that's making tests fail (though now there will be some conflicts to fix up since it's been a bit). L178 of tests/stdlib/tos.nim

tests/stdlib/tos.nim

timotheecour · 2018-09-30T08:48:20Z

this is waiting on #9116 which I need in implementation

timotheecour · 2018-10-10T21:00:48Z

PTAL, all comments addressed
/cc @Araq I revived this now that lastPathPart was merged.

* fix nim-lang#8225 isHidden was broken on posix * scope rest of tos.nim under blocks to avoid variable scope bugs

timotheecour force-pushed the pr_isHidden branch 3 times, most recently from 688fc2d to 47f6193 Compare July 14, 2018 08:05

timotheecour mentioned this pull request Jul 14, 2018

[ospaths, os] miscellaneous issues #8177

Open

26 tasks

Varriount requested review from dom96 and Varriount July 14, 2018 10:43

dom96 reviewed Jul 14, 2018

View reviewed changes

timotheecour added a commit to timotheecour/Nim that referenced this pull request Jul 14, 2018

add a sample function that can serve as reference for code to follow …

b370902

…(adapted from nim-lang#8315 (comment))

dom96 reviewed Jul 15, 2018

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

dom96 reviewed Jul 15, 2018

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

haltcase mentioned this pull request Jul 16, 2018

includeHidden=false doesn't prevent recursion into hidden directories haltcase/glob#7

Closed

haltcase reviewed Jul 16, 2018

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

timotheecour force-pushed the pr_isHidden branch from 4679ec2 to 2b5e80c Compare July 17, 2018 07:51

timotheecour mentioned this pull request Jul 17, 2018

[RFC] extractFilename("usr/lib/") should return "lib" (not "") and be called baseName (since works with dirs) #8341

Closed

timotheecour mentioned this pull request Jul 30, 2018

[WIP] [doc] miscellaneous issues #8477

Closed

21 tasks

haltcase suggested changes Sep 27, 2018

View reviewed changes

tests/stdlib/tos.nim Outdated Show resolved Hide resolved

timotheecour force-pushed the pr_isHidden branch 2 times, most recently from 350c91c to 3a0ac8d Compare October 10, 2018 20:48

timotheecour closed this Oct 11, 2018

timotheecour reopened this Oct 11, 2018

timotheecour force-pushed the pr_isHidden branch from 1766317 to 89f7f95 Compare October 12, 2018 19:21

timotheecour added 2 commits October 13, 2018 00:36

fix nim-lang#8225 isHidden was broken on posix

34b2f53

scope rest of tos.nim under blocks to avoid variable scope bugs

c73ca78

timotheecour force-pushed the pr_isHidden branch from 89f7f95 to c73ca78 Compare October 13, 2018 07:37

Araq merged commit 3bef851 into nim-lang:devel Oct 14, 2018

timotheecour deleted the pr_isHidden branch October 14, 2018 23:16

krux02 pushed a commit to krux02/Nim that referenced this pull request Oct 15, 2018

fix nim-lang#8225 os.isHidden was buggy on posix (nim-lang#8315)

189971a

* fix nim-lang#8225 isHidden was broken on posix * scope rest of tos.nim under blocks to avoid variable scope bugs

narimiran pushed a commit to narimiran/Nim that referenced this pull request Oct 31, 2018

fix nim-lang#8225 os.isHidden was buggy on posix (nim-lang#8315)

1a147e8

* fix nim-lang#8225 isHidden was broken on posix * scope rest of tos.nim under blocks to avoid variable scope bugs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #8225 os.isHidden was buggy on posix #8315

fix #8225 os.isHidden was buggy on posix #8315

timotheecour commented Jul 14, 2018 •

edited

Loading

Varriount commented Jul 14, 2018

timotheecour commented Jul 14, 2018 •

edited

Loading

dom96 Jul 14, 2018

timotheecour Jul 14, 2018 •

edited

Loading

dom96 Jul 14, 2018 •

edited

Loading

timotheecour Jul 14, 2018

dom96 Jul 14, 2018

timotheecour Jul 14, 2018

timotheecour Jul 14, 2018

timotheecour Jul 14, 2018 •

edited

Loading

dom96 Jul 14, 2018

timotheecour Jul 14, 2018

timotheecour commented Jul 14, 2018

dom96 Jul 15, 2018

timotheecour Jul 17, 2018

dom96 Jul 15, 2018

timotheecour Jul 17, 2018

Varriount commented Jul 21, 2018

haltcase left a comment •

edited

Loading

timotheecour commented Sep 30, 2018

timotheecour commented Oct 10, 2018 •

edited

Loading

fix #8225 os.isHidden was buggy on posix #8315

fix #8225 os.isHidden was buggy on posix #8315

Conversation

timotheecour commented Jul 14, 2018 • edited Loading

Varriount commented Jul 14, 2018

timotheecour commented Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

timotheecour Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

dom96 Jul 14, 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

timotheecour Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Jul 14, 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

Varriount commented Jul 21, 2018

haltcase left a comment • edited Loading

Choose a reason for hiding this comment

timotheecour commented Sep 30, 2018

timotheecour commented Oct 10, 2018 • edited Loading

timotheecour commented Jul 14, 2018 •

edited

Loading

timotheecour commented Jul 14, 2018 •

edited

Loading

timotheecour Jul 14, 2018 •

edited

Loading

dom96 Jul 14, 2018 •

edited

Loading

timotheecour Jul 14, 2018 •

edited

Loading

haltcase left a comment •

edited

Loading

timotheecour commented Oct 10, 2018 •

edited

Loading