-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Windows-specific bugs #341
base: main
Are you sure you want to change the base?
Conversation
`os.path.join` uses an OS-dependent separator (i.e. backslash on Windows, which leads to invalid URLs); let’s hardcode a forward slash here. Later, ModelRegistry.get_file() turns this string into a Path in https://github.com/SNEWS2/snewpy/blob/50ba643f3e5779b11de06d6904748a9a2557e2ce/python/snewpy/_model_downloader.py#L194 so the OS-specific separator is then used for the local file.
The first commit (50ba643) adds test for Windows[*] without any fixes to the code; to confirm that the tests fail in CI and reproduce the issue observed by our project student. The second commit (22b66ce) adds a fix for the Now the only remaining test failure is in [*] and MacOS, for good measure; though that is a UNIX and shouldn’t have as many peculiarities |
Hm … one remaining issue in the integration tests: The command I see a few options:
Option 1 would be the right thing to do; but since I don’t have access to a Windows machine, that doesn’t sound like a rabbit hole I particularly want to go down … 🫣 |
PowerShell is not expanding wildcard characters, but requires separate command for that via https://stackoverflow.com/questions/43897242/powershell-wildcards-in-passing-filenames-as-arguments
2871549
to
c713c78
Compare
After some experimentation, it turns out that PowerShell apparently does not expand wildcard characters (which is not obvious from the documentation 🤬). Commit c713c78 adds the additional code required for this. With that, afaict the test on Windows are equivalent to those on Linux/MacOS and this PR is ready for review. |
This should eventually fix #340.