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

fix(bindings/python): missed to call close for the file internally #4122

Merged
merged 3 commits into from
Feb 1, 2024
Merged

fix(bindings/python): missed to call close for the file internally #4122

merged 3 commits into from
Feb 1, 2024

Conversation

zzl221000
Copy link
Contributor

close writer
fixes #4120

@Xuanwo
Copy link
Member

Xuanwo commented Feb 1, 2024

clippy is the rust lint that help us write better code, for example:

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
   --> bindings/python/src/file.rs:399:13
    |
399 | /             match &mut *state {
400 | |                 AsyncFileState::Writer(w) => {
401 | |                     w.close()
402 | |                         .await
...   |
405 | |                 _ => {}
406 | |             }
    | |_____________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match
help: try
    |
399 ~             if let AsyncFileState::Writer(w) = &mut *state {
400 +                 w.close()
401 +                     .await
402 +                     .map_err(|err| PyIOError::new_err(err.to_string()))?;
403 +             }

@@ -185,7 +192,7 @@ impl File {
}

pub fn __exit__(&mut self, _exc_type: PyObject, _exc_value: PyObject, _traceback: PyObject) {
self.0 = FileState::Closed;
let _ = self.close();
Copy link
Member

Choose a reason for hiding this comment

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

Hi, @messense @Zheaoli is there a way for use to return error during exist context manager?

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo
Copy link
Member

Xuanwo commented Feb 1, 2024

We need to fix some edge cases, but this PR seems to be an improvement to the current codebase. Let's merge it first!

@Xuanwo Xuanwo merged commit f05b94c into apache:main Feb 1, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The S3 file like writer in Python is not completing correctly.
2 participants