[internal] Inline some of externs/mod.rs and make GIL acquisition explicit
#13522
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This is more boilerplate, but makes clear at call sites that the GIL is held during Eq # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| match externs::getattr_as_optional_string(py, value, "working_directory") { | ||
| None => None, | ||
| Some(dir) => Some(RelativePath::new(dir)?), | ||
| }; | ||
|
|
There was a problem hiding this comment.
Just use .map(|s| RelativePath::new(s)?)
There was a problem hiding this comment.
I tried, but complains that the closure isn't returning Option or Result, so the ? doesn't work. The only way I could get things to compile is RelativePath::new().unwrap(), which is a downgrade from before
| externs::getattr_as_string(&externs::getattr(value, "cache_scope").unwrap(), "name") | ||
| .try_into()?; | ||
| let cache_scope: ProcessCacheScope = { | ||
| let cache_scope_enum: PyObject = externs::getattr(value, "cache_scope").unwrap(); |
There was a problem hiding this comment.
Maybe avoid the unwrap? .ok_or_else(|| "Missing cache_scope attribute".into())?
There was a problem hiding this comment.
Similar comment applies to other locations with .unwrap
There was a problem hiding this comment.
I think that might be even more confusing. Really, this should not be calling .unwrap() and should propagate the error. But error handling is clunky in rust-cpython (much better with PyO3). My main focus is getting PyO3 working with making as few changes as feasible. Then, once PyO3 lands, we can start improving the code like this.
|
|
||
| let glob_match_error_behavior: PyObject = | ||
| externs::getattr(item, "glob_match_error_behavior").unwrap(); | ||
| let failure_behavior = externs::getattr_as_string(&glob_match_error_behavior, "value"); | ||
| let failure_behavior: String = externs::getattr(&glob_match_error_behavior, "value").unwrap(); |
There was a problem hiding this comment.
.ok_or_else(|| "Some message".into())?
A couple of refactors to simplify our rust-cpython FFI code.
Most notable is rewriting
gettattr_as_stringto begetattr_as_optional_string, which is always how it was behaving. That is, we used to tolerate if Python was set tostr | None, even though most call sites expectedstr. This rename makes clear the difference.