refactor: Remove all usage of custom OSName class #203
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mwiebe
force-pushed
the
remove-custom-osname
branch
6 times, most recently
from
4fd4ffc to
667c001
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mwiebe
force-pushed
the
remove-custom-osname
branch
3 times, most recently
from
f4c4767 to
8afeab6
Compare
crowecawcaw
previously approved these changes
Jun 16, 2025
mwiebe
force-pushed
the
remove-custom-osname
branch
from
8afeab6 to
2046441
Compare
crowecawcaw
previously approved these changes
Jun 16, 2025
epmog
reviewed
Jun 19, 2025
* Instead of the custom OSName, use Python idioms. This will be easier
to understand for Python developers and tooling that is processing the
code.
1. For Windows: os.name == "nt"
2. For POSIX: os.name != "nt"
3. For MacOS: sys.platform == "darwin"
4. For a human-readable OS description: platform.platform()
* Comments '# pragma: is-windows' were unclear, it required
investigation to learn that this is a coverage directive. Rename the
directive to '# pragma: skip-coverage-windows'
Signed-off-by: Mark <[email protected]>
mwiebe
force-pushed
the
remove-custom-osname
branch
from
2046441 to
dcae681
Compare
|
crowecawcaw
approved these changes
Jun 19, 2025
epmog
reviewed
Jun 19, 2025
| self._source_path_format: str = source_path_format | ||
|
|
||
| destination_os = destination_os.upper() | ||
| if destination_os not in ("WINDOWS", "POSIX"): |
Contributor
There was a problem hiding this comment.
do we need to temporarily accept "Linux"/"Macos" as well (whatever the previous values were)? otherwise, previous rules may now raise ValueErrors
Contributor
Author
There was a problem hiding this comment.
Both of those should map to POSIX, you're right that it was doing something different before. I'll do a quick survey of the deadline-cloud-for-* projects to get a sense of usage, likely we accept them as input and transform them into "POSIX".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the problem/requirement? (What/Why)
The library implements a custom OSName for checking which OS it's running on and
reporting in error messages. It also contains '#pragma is-windows' and similar comments
that a code reader will misinterpret.
What was the solution? (How)
What is the impact of this change?
The code is easier to work with and maintain because it follows Python idioms more closely.
How was this change tested?
Ran the unit test.
As a follow-up test, I ran the deadline-cloud-for-blender integration tests with this library (by running pytest directly in an environment including both instead of relying on hatch). See the related aws-deadline/deadline-cloud-for-blender#241 that I produced in the course of that.
Was this change documented?
No
Is this a breaking change?
No. The OSName class is still in the code, it is commented as deprecated, and the implementation no longer uses it.
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.