-
Notifications
You must be signed in to change notification settings - Fork 86
chore: deny let_underscore_drop #5095
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| #![allow(clippy::unwrap_used)] | ||
| #![allow(clippy::expect_used)] | ||
| use std::io::ErrorKind; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::{env, fs}; | ||
|
|
||
|
|
@@ -44,7 +45,10 @@ fn download_duckdb_lib_archive() -> Result<PathBuf, Box<dyn std::error::Error>> | |
| let archive_path = duckdb_dir.join(&archive_name); | ||
|
|
||
| // Recreate the duckdb directory | ||
| let _ = fs::remove_dir_all(&duckdb_dir); | ||
| match fs::remove_dir_all(&duckdb_dir) { | ||
| Err(err) if err.kind() == ErrorKind::NotFound => (), | ||
| otherwise => otherwise?, | ||
| } | ||
| fs::create_dir_all(&duckdb_dir)?; | ||
|
|
||
| if !archive_path.exists() { | ||
|
|
@@ -192,7 +196,10 @@ fn build_duckdb(duckdb_source_root: &Path) -> Result<PathBuf, Box<dyn std::error | |
| let target_dir = manifest_dir.parent().unwrap().join("target"); | ||
| let duckdb_library_dir = target_dir.join("duckdb-lib"); | ||
|
|
||
| let _ = fs::remove_dir_all(&duckdb_library_dir); | ||
| match fs::remove_dir_all(&duckdb_library_dir) { | ||
| Err(err) if err.kind() == ErrorKind::NotFound => (), | ||
| otherwise => otherwise?, | ||
| } | ||
| fs::create_dir_all(&duckdb_library_dir)?; | ||
|
|
||
| // Copy .dylib and .so files (macOS and Linux). | ||
|
|
@@ -223,7 +230,7 @@ fn main() { | |
| // Download, extract and symlink DuckDB source code. | ||
| let zip_source_path = download_duckdb_source_archive().unwrap(); | ||
| let extracted_source_path = extract_duckdb_source(zip_source_path).unwrap(); | ||
| let _ = fs::remove_dir_all(&duckdb_repo); | ||
| drop(fs::remove_dir_all(&duckdb_repo)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unwrap the result
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with the following because I think the intention is to not fail if the directory already exists (but I agree we should fail if there's, e.g. a permissions issue or some other weird problem). |
||
| std::os::unix::fs::symlink(&extracted_source_path, &duckdb_repo).unwrap(); | ||
|
|
||
| let library_path = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,6 +135,7 @@ mod tests { | |
| use vortex_array::arrays::StructArray; | ||
| use vortex_buffer::buffer; | ||
| use vortex_dtype::{DType, Nullability, PType}; | ||
| use vortex_error::VortexUnwrap as _; | ||
|
|
||
| use super::cast; | ||
| use crate::exprs::get_item::get_item; | ||
|
|
@@ -155,7 +156,7 @@ mod tests { | |
| #[test] | ||
| fn replace_children() { | ||
| let expr = cast(root(), DType::Bool(Nullability::Nullable)); | ||
| let _ = expr.with_children([root()]); | ||
| expr.with_children(vec![root()]).vortex_unwrap(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This did not test what it claimed to test. |
||
| } | ||
|
|
||
| #[test] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,7 +124,7 @@ impl Drop for ReadFuture { | |
| fn drop(&mut self) { | ||
| // When the FileHandle is dropped, we can send a shutdown event to the I/O stream. | ||
| // If the I/O stream has already been dropped, this will fail silently. | ||
| let _ = self.events.unbounded_send(ReadEvent::Dropped(self.id)); | ||
| drop(self.events.unbounded_send(ReadEvent::Dropped(self.id))); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should also unwrap the result i think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh i see, we don't want to panic in the drop impl here. |
||
| } | ||
| } | ||
|
|
||
|
|
||
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 a bit suspicious to me? Shouldn't we fail if the external table fails to create?
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.
isn't that handled by the
.await??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.
Ah, yes, you're right. This is flagged because DataFrame has a non-trivial Drop (probably drops some Arcs).