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

Account for non-UNIX platforms in sys.platform checks #3722

Merged
merged 2 commits into from Mar 14, 2022

Conversation

joshuacwnewton
Copy link
Member

Otherwise, we'd get a "referenced before assignment" error on
Windows.

Checklist

GitHub

PR contents

Description

This PR fills in the gaps for platforms outside of darwin and linux.

I think this check probably could use a more thorough rewrite, though? I'm skeptical of its purpose.

Linked issues

Fixes #3709.

Otherwise, we'd get a "referenced before assignment" error on
Windows.
@joshuacwnewton joshuacwnewton added bug category: fixes an error in the code sct_run_batch context: labels Mar 7, 2022
@joshuacwnewton joshuacwnewton added this to the 5.6 milestone Mar 7, 2022
@joshuacwnewton joshuacwnewton self-assigned this Mar 7, 2022
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #3722 (9163f28) into master (2c2a662) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Flag Coverage Δ
api-tests 22.30% <0.00%> (-2.16%) ⬇️
cli-tests 58.74% <0.00%> (+1.80%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
spinalcordtoolbox/scripts/sct_run_batch.py 70.95% <0.00%> (-0.90%) ⬇️

Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This check seems to be purely for printing diagnostic information to the logs, so it seems fine.

@joshuacwnewton joshuacwnewton merged commit 8c7413b into master Mar 14, 2022
@joshuacwnewton joshuacwnewton deleted the jn/3709-fixup-sys.platform-checks branch March 14, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category: fixes an error in the code OS: Windows (native) sct_run_batch context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include win32 in OS-specific checks that involve sys.platform
2 participants