Skip to content
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

Handle definitions @import from relative paths on Windows #1508

Merged
merged 4 commits into from
Apr 19, 2022

Conversation

khaeru
Copy link
Contributor

@khaeru khaeru commented Apr 11, 2022

Initially as suggested at #1498 (comment), since this continues to affect downstream packages and their CI on Windows.

The actual fix ended up being using pathlib.Path for ImportDefinition.path; this way its str() representation can be compared with those of other Path objects, and they will be consistent on any given OS.

I realized that the pint test suite does not run on Windows; this is one reason the particular manifestation of the bug mentioned at #1498 (comment) was not noticed in #1499. So, I added (probably in the wrong place/form, sorry) a slimmed-down copy of the test-linux jobs, named test-windows, to confirm.

Happy to make any desired adjustments.

@khaeru khaeru force-pushed the issue/1498 branch 5 times, most recently from d4aba76 to e446a97 Compare April 11, 2022 09:25
khaeru added 2 commits April 11, 2022 11:30
Use provided .testsuite.helpers.require_numpy instead of enclosing tests in an
if block.
@khaeru
Copy link
Contributor Author

khaeru commented Apr 11, 2022

The GHA workflows appear to run under my fork (neat), so here's a confirmation of the bug occurring on the Windows runners: https://github.com/khaeru/pint/runs/5969710141?check_suite_focus=true#step:9:161

I'll now remove the "TEMPORARY" commits from the branch.

@khaeru khaeru marked this pull request as draft April 11, 2022 09:41
@khaeru
Copy link
Contributor Author

khaeru commented Apr 11, 2022

Now seeing this:

 E                   ValueError: The order of the files do not match. (expected: C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\test_issue14980\def2.txt, found C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\test_issue14980\def2.txt)

These are the same files; nothing to do with subdirectories.

Will experiment further!

@khaeru khaeru marked this pull request as ready for review April 11, 2022 09:55
@khaeru
Copy link
Contributor Author

khaeru commented Apr 11, 2022

khaeru added a commit to khaeru/genno that referenced this pull request Apr 13, 2022
khaeru added a commit to khaeru/genno that referenced this pull request Apr 13, 2022
khaeru added a commit to khaeru/genno that referenced this pull request Apr 13, 2022
@hgrecco hgrecco merged commit f0f9002 into hgrecco:master Apr 19, 2022
@hgrecco hgrecco mentioned this pull request Apr 21, 2022
khaeru added a commit to khaeru/genno that referenced this pull request Apr 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New base units not usable in @context in @import-ed file
2 participants