katzen-cafe/iowo
2
4
Fork 1
Code Issues 4 Pull requests 2 Projects Releases Packages Wiki Activity

WIP: image processing library (or libraries?) #12

Draft
schrottkatze wants to merge 15 commits from schrottkatze/iowo:proc-libs into main
pull from: schrottkatze/iowo:proc-libs
merge into: katzen-cafe:main
Conversation 3 Commits 15 Files changed 20 +4 -2
2 changed files with 4 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 062138337c - Show all commits

4
crates/prowocessing/src/experimental/trait_based/data/io.rs
View file

@ -5,6 +5,7 @@ use std::{borrow::ToOwned, convert::Into};
use super::raw::Data; use super::raw::Data;
/// Newtype struct with borrowed types for pipeline/element inputs, so that doesn't force a move or clone /// Newtype struct with borrowed types for pipeline/element inputs, so that doesn't force a move or clone
schrottkatze marked this conversation as resolved
multisamplednight commented 2024-02-22 17:35:57 +00:00
Review
Copy link

Quite ambiguous doc-comment also regarding the rather lengthy doc-comment on the type itself.
How about removing this method altogether and making the content of Inputs directly public,
given that one's free to convert from/to it already?

Quite ambiguous doc-comment also regarding the rather lengthy doc-comment on the type itself. How about removing this method altogether and making the content of `Inputs` directly public, given that one's free to convert from/to it already?
#[derive(PartialEq, Eq, Debug)]
pub struct Inputs<'a>(pub Vec<&'a Data>); pub struct Inputs<'a>(pub Vec<&'a Data>);
impl<'a> From<Vec<&'a Data>> for Inputs<'a> { impl<'a> From<Vec<&'a Data>> for Inputs<'a> {
@ -26,7 +27,8 @@ impl<'a> From<&'a Outputs> for Inputs<'a> {
} }
schrottkatze marked this conversation as resolved
multisamplednight commented 2024-02-22 17:37:23 +00:00
Review
Copy link

Unnecessary full method path, consider just using From::from or Into::into instead.

Unnecessary full method path, consider just using `From::from` or `Into::into` instead.
schrottkatze commented 2024-02-23 09:37:03 +00:00
Review
Copy link

ah yes, rust-analyzer loves completing full paths lol

ah yes, rust-analyzer loves completing full paths lol
/// Used for pipeline/element outputs /// Used for pipeline/element outputs
pub struct Outputs(Vec<Data>); #[derive(PartialEq, Eq, Debug)]
pub struct Outputs(pub Vec<Data>);
schrottkatze marked this conversation as resolved Outdated
multisamplednight commented 2024-02-22 17:36:44 +00:00
Outdated
Review
Copy link

I already see that this is a newtype around OwnedData, just re-stating it does not add semantical value. Rather, what would this struct be used for?

I already see that this is a newtype around `OwnedData`, just re-stating it does not add semantical value. Rather, what would this struct be used for?
impl Outputs { impl Outputs {
/// consume self and return inner value(s) /// consume self and return inner value(s)

2
crates/prowocessing/src/experimental/trait_based/data/raw.rs
View file

@ -1,7 +1,7 @@
//! Dynamic data storage and transfer types for use in [`io`] //! Dynamic data storage and transfer types for use in [`io`]
// Dynamic data type // Dynamic data type
#[derive(Clone, Debug)] #[derive(Clone, Debug, PartialEq, Eq)]
pub enum Data { pub enum Data {
schrottkatze marked this conversation as resolved Outdated
multisamplednight commented 2024-02-22 17:40:30 +00:00
Outdated
Review
Copy link

Given the common pattern of AsRef in the stdlib and InstructionRef already being used in this project, how about renaming OwnedData → Data and Data below → DataRef?

Given the common pattern of `AsRef` in the stdlib and `InstructionRef` already being used in this project, how about renaming `OwnedData` → `Data` and `Data` below → `DataRef`?
schrottkatze commented 2024-02-23 09:43:36 +00:00
Outdated
Review
Copy link

did the renaming, gonna take care of the AsRef impl later (I remember trying that earlier and something causing problems, i forgot what that was though.)

did the renaming, gonna take care of the `AsRef` impl later (I remember trying that earlier and something causing problems, i forgot what that was though.)
String(String), String(String),
Int(i32), Int(i32),