Skip to content

Commit faa9b84

Browse files
dbrattliclaude
andcommitted
fix(stdlib): address review feedback on Os bindings
- Change os.path.getsize return type from int to int64 to handle files >= 2 GiB - Reorder getpid/getppid next to getenv; move walk near rmdir for grouping - Improve walk test to assert dirpath="." and that subdirs+files match listdir - Add getppid test - Add walk(topdown=false) test using a controlled temp dir - Use unique /tmp paths and clean up after makedirs/walk tests - Revert CHANGELOG.md addition (AGENTS.md forbids modifying CHANGELOG) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bbe0ce2 commit faa9b84

3 files changed

Lines changed: 46 additions & 26 deletions

File tree

CHANGELOG.md

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,6 @@ All notable changes to this project will be documented in this file.
4646

4747
## Unreleased
4848

49-
### 🚀 Features
50-
51-
* *(stdlib)* Add `os.makedirs(path, exist_ok)` overload, `os.walk`, `os.getpid`, `os.getppid`, `os.path.isabs`, `os.path.islink`, `os.path.realpath`, `os.path.getsize` to Os module
52-
5349
### 🐞 Bug Fixes
5450

5551
* Fix `math.factorial` binding: changed signature from `float -> float` to `int -> int` to match Python 3.12+ where float arguments raise `TypeError`. Fixes test to use integer literals.

src/stdlib/Os.fs

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@ type IExports =
3030
/// Return the value of the environment variable key or default if not set
3131
/// See https://docs.python.org/3/library/os.html#os.getenv
3232
abstract getenv: key: string * ``default``: string -> string
33+
/// Return the current process id.
34+
/// See https://docs.python.org/3/library/os.html#os.getpid
35+
abstract getpid: unit -> int
36+
/// Return the parent process id.
37+
/// See https://docs.python.org/3/library/os.html#os.getppid
38+
abstract getppid: unit -> int
3339
/// Send signal sig to the process pid
3440
/// See https://docs.python.org/3/library/os.html#os.kill
3541
abstract kill: pid: int * ``sig``: int -> unit
@@ -51,21 +57,6 @@ type IExports =
5157
/// See https://docs.python.org/3/library/os.html#os.makedirs
5258
[<Emit("$0.makedirs($1, $2, $3)")>]
5359
abstract makedirs: path: string * mode: int * exist_ok: bool -> unit
54-
/// Return the current process id.
55-
/// See https://docs.python.org/3/library/os.html#os.getpid
56-
abstract getpid: unit -> int
57-
/// Return the parent process id.
58-
/// See https://docs.python.org/3/library/os.html#os.getppid
59-
abstract getppid: unit -> int
60-
/// Walk a directory tree, yielding (dirpath, dirnames, filenames) for each directory.
61-
/// When topdown is true (the default) the caller can modify the dirnames list in-place
62-
/// to prune the search or impose a specific visiting order.
63-
/// See https://docs.python.org/3/library/os.html#os.walk
64-
abstract walk: top: string -> seq<string * ResizeArray<string> * ResizeArray<string>>
65-
/// Walk a directory tree top-down or bottom-up (topdown=false).
66-
/// See https://docs.python.org/3/library/os.html#os.walk
67-
[<Emit("$0.walk($1, topdown=$2)")>]
68-
abstract walk: top: string * topdown: bool -> seq<string * ResizeArray<string> * ResizeArray<string>>
6960
/// Set the environment variable named key to the string value
7061
/// See https://docs.python.org/3/library/os.html#os.putenv
7162
abstract putenv: key: string * value: string -> unit
@@ -81,6 +72,15 @@ type IExports =
8172
/// Remove (delete) the directory path
8273
/// See https://docs.python.org/3/library/os.html#os.rmdir
8374
abstract rmdir: path: string -> unit
75+
/// Walk a directory tree, yielding (dirpath, dirnames, filenames) for each directory.
76+
/// When topdown is true (the default) the caller can modify the dirnames list in-place
77+
/// to prune the search or impose a specific visiting order.
78+
/// See https://docs.python.org/3/library/os.html#os.walk
79+
abstract walk: top: string -> seq<string * ResizeArray<string> * ResizeArray<string>>
80+
/// Walk a directory tree top-down or bottom-up (topdown=false).
81+
/// See https://docs.python.org/3/library/os.html#os.walk
82+
[<Emit("$0.walk($1, topdown=$2)")>]
83+
abstract walk: top: string * topdown: bool -> seq<string * ResizeArray<string> * ResizeArray<string>>
8484
/// Test whether a path exists
8585
/// See https://docs.python.org/3/library/os.path.html#os.path.exists
8686
abstract path: PathModule
@@ -128,7 +128,7 @@ and [<Erase>] PathModule =
128128
abstract realpath: path: string -> string
129129
/// Return the size, in bytes, of path
130130
/// See https://docs.python.org/3/library/os.path.html#os.path.getsize
131-
abstract getsize: path: string -> int
131+
abstract getsize: path: string -> int64
132132

133133

134134
/// Miscellaneous operating system interfaces

test/TestOs.fs

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,26 +80,50 @@ let ``test os.path.islink works`` () =
8080
[<Fact>]
8181
let ``test os.path.getsize works`` () =
8282
// The test directory itself has a positive size
83-
os.path.getsize "." > 0 |> equal true
83+
os.path.getsize "." > 0L |> equal true
8484

8585
[<Fact>]
8686
let ``test os.getpid works`` () =
8787
let pid = os.getpid ()
8888
pid > 0 |> equal true
8989

90+
[<Fact>]
91+
let ``test os.getppid works`` () =
92+
let ppid = os.getppid ()
93+
ppid > 0 |> equal true
94+
9095
[<Fact>]
9196
let ``test os.makedirs with exist_ok works`` () =
92-
let dir = "/tmp/fable_test_makedirs"
97+
let dir = sprintf "/tmp/fable_test_makedirs_%d" (os.getpid ())
9398
os.makedirs (dir, true)
9499
os.path.isdir dir |> equal true
95100
// Second call must not raise when exist_ok=true
96101
os.makedirs (dir, true)
97102
os.path.isdir dir |> equal true
103+
os.rmdir dir
98104

99105
[<Fact>]
100106
let ``test os.walk yields entries`` () =
101107
let entries = os.walk "." |> Seq.truncate 1 |> Seq.toList
102-
// At minimum one entry (the root ".")
103-
entries.Length > 0 |> equal true
104-
let _dirpath, _subdirs, _files = entries.[0]
105-
true |> equal true
108+
entries.Length |> equal 1
109+
let dirpath, subdirs, files = entries.[0]
110+
dirpath |> equal "."
111+
// The first walk entry's dirnames + filenames should equal listdir of the root
112+
let walkAll = Seq.append subdirs files |> Seq.sort |> Seq.toList
113+
let listdirAll = os.listdir "." |> Array.sort |> Array.toList
114+
walkAll |> equal listdirAll
115+
116+
[<Fact>]
117+
let ``test os.walk with topdown=false works`` () =
118+
let root = sprintf "/tmp/fable_test_walk_%d" (os.getpid ())
119+
let nested = os.path.join (root, "nested")
120+
os.makedirs (nested, true)
121+
let entries = os.walk (root, false) |> Seq.toList
122+
// Bottom-up: deepest dir is yielded first, root is yielded last
123+
entries.Length |> equal 2
124+
let firstDir, _, _ = entries.[0]
125+
let lastDir, _, _ = List.last entries
126+
firstDir |> equal nested
127+
lastDir |> equal root
128+
os.rmdir nested
129+
os.rmdir root

0 commit comments

Comments
 (0)