Repository navigation
fix: escape Windows backslashes in Julia path strings - #85
ghita-el-amlaqui wants to merge 2 commits into
Conversation
|
@ghita-el-amlaqui Thanks! Could you please also run the pytests on your Windows machine and check that they all pass? pip install -e ".[dev,test]"
pytest tests/ -n <number_of_cores_of_your_machine> -v |
|
@ghita-el-amlaqui Thanks for the fix — normalising the paths to forward slashes before embedding them in the Julia snippet is a clean, targeted solution, and it's nicely contained (one file, clear comment). What's needed
Everything else looks good — all checks are green and the branch is clean, so no rebase needed. I'll leave the merge call to a maintainer. — 🤖 _automated pre-review; a maintainer will follow up_ |
|
@ghita-el-amlaqui Quick follow-up — Two things:
I'll leave the merge/DCO remediation call to a maintainer. — 🤖 _automated pre-review; a maintainer will follow up_ |
Hello @ghita-el-amlaqui , could you please check this? thank you! |
|
@ghita-el-amlaqui Quick update — good news since my last note: the DCO check is now passing, so the So the one thing @albanpuech is (rightly) waiting on is the Windows-side confirmation — since CI only runs on Linux, it can't exercise the Windows path your change actually fixes: pip install -e ".[dev,test]"
pytest tests/ -n <cores> -vA short paste of the passing summary would let a maintainer wrap this up. (The optional no-backslash regression test is still just nice-to-have, not blocking.) — 🤖 _automated pre-review; a maintainer will follow up_ |
|
Hello @romeokienzler, here's a recap of the tests I ran on Windows:
Error messages: I'll include the fixed code in the new PR. |
|
@ghita-el-amlaqui Thanks for running the full suite on Windows and pasting the summary — that's exactly the confirmation we needed. The key point for this PR: the Windows parse that your change fixes now works end-to-end (install succeeded, 239 passed), so the backslash→forward-slash normalisation does its job. On the two failures — neither looks related to your diff:
So I wouldn't hold this PR on them. Fixing those Windows-specific test issues in a separate PR (as you suggested) is the right call — keeps this fix small and reviewable. One housekeeping item: the branch now shows as I'll leave the merge decision to a maintainer. — 🤖 _automated pre-review; a maintainer will follow up_ |
6c8bb8d to
6f1342f
Compare
Signed-off-by: ghita-el-amlaqui <ghita.el.amlaqui@ibm.com>
|
@ghita-el-amlaqui Thanks for pushing the fixes — and for signing off the commit (DCO is green). Checking the new commit (
Both are isolated to One small note: I'd earlier suggested landing these in a separate PR to keep this one minimal — but folding them in here is a perfectly reasonable call too, since they're small and directly unblock your Windows run. Entirely your preference; I don't think it's worth splitting now. If you can re-run |
Fix: Resolve two Windows-specific test failuresThis PR fixes also the two test failures that only manifest on Windows: 1.
|
|
@ghita-el-amlaqui Perfect — that closes the loop. The re-run on Windows going from 2 failed → 0 failed (239 passed, 361 skipped) confirms both test fixes do their job: the SHA-1 short-hash dir name dodges the MAX_PATH limit, and the So from a pre-review standpoint this now looks complete:
Looks ready for a maintainer's look. Merge call stays with a maintainer. — 🤖 _automated pre-review; a maintainer will follow up_ |
Fix: Escape Windows backslashes in Julia path strings
Problem
Running
gridfm_datakit generateon Windows fails with a JuliaParseError:Windows paths embedded directly into Julia source code as f-string literals are parsed by Julia as containing invalid Unicode escape sequences (e.g.
\U,\A), crashing before any OPF solve can begin.Fix
Normalise both paths to forward slashes before constructing the Julia code snippet:
Julia and PowerModels accept forward-slash paths on Windows. Behaviour on Linux/macOS is unchanged.
Files changed
gridfm_datakit/network.pyType of change