-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Remove importlib frames from traceback #1104
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
Remove importlib frames from traceback #1104
Conversation
This is a adaptation of the |
@@ -144,3 +144,35 @@ fn find_source(vm: &VirtualMachine, current_path: PathBuf, name: &str) -> Result | |||
None => Err(format!("No module named '{}'", name)), | |||
} | |||
} | |||
|
|||
pub fn remove_importlib_frames(vm: &VirtualMachine, exc: &PyObjectRef) -> PyObjectRef { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to add this? This might be a good opportunity to keep the rustpython interpreter transparent to users, and show them what is actually going on. What is your opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it gives too much information. I will add a TODO
that when running on verbose
this function will do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for a minor comment, this looks good to me!
vm/src/import.rs
Outdated
if let Ok(tb) = vm.get_attribute(exc.clone(), "__traceback__") { | ||
if objtype::isinstance(&tb, &vm.ctx.list_type()) { | ||
let mut tb_entries = objsequence::get_elements_list(&tb).to_vec(); | ||
tb_entries.reverse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the list is reversed twice. This might not be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
vm/src/import.rs
Outdated
if objtype::isinstance(&tb, &vm.ctx.list_type()) { | ||
let mut tb_entries = objsequence::get_elements_list(&tb).to_vec(); | ||
tb_entries.reverse(); | ||
let mut new_tb = Vec::with_capacity(tb_entries.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this function is basically a filtering of a list, this might be an excellent opportunity to use the filter_map
function, or filter
function on rust iterators, and then use collect
on the filtered iterator.
Something along this idea:
let new_tb = tb_entries.iter()
.filter(|tb_entry| {
// fetch filename etc..
// funky filtering here, if file_name != frozen etc...
})
.collect();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only a simple filter if the error is from outside the importlib
code. That is why we remove only chunks that end in _call_with_frames_removed
.
vm/src/import.rs
Outdated
new_tb.push(tb_entry.clone()) | ||
} else { | ||
current_chunk.push(tb_entry.clone()); | ||
let run_obj_name = objstr::get_value(&location_attrs[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as file_name
earlier on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Thanks.
vm/src/import.rs
Outdated
let mut new_tb = Vec::with_capacity(tb_entries.len()); | ||
|
||
for tb_entry in tb_entries.iter() { | ||
let mut current_chunk = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this variable can be removed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
c9f01cc
to
251e33a
Compare
Previously the traceback was:
Now it is: