chanced / jsonptr

JSON Pointer (RFC 6901) implementation for Rust
Apache License 2.0
44 stars 4 forks source link

`PointerBuf::parse` accepts `Into<String>` but does not include the `String` in the `Err` #96

Open chanced opened 1 day ago

chanced commented 1 day ago

We likely take ownership of the input to PointerBuf::parse but do not include it in resulting ParseError.

PointerBuf::parse:

pub fn parse(s: impl Into<String>) -> Result<Self, ParseError> {
    let s = s.into();
    validate(&s)?;
    Ok(Self(s))
}

ParseError:

pub enum ParseError {
    NoLeadingBackslash,
    InvalidEncoding {
        offset: usize,
        source: InvalidEncodingError,
    },
}
chanced commented 1 day ago

@asmello referencing my comment in our conversation over on the error pull request https://github.com/chanced/jsonptr/pull/93#discussion_r1832748163

chanced commented 1 day ago

This is bleed-over from our discussion in #93 but this is what I had in mind for Report:


#[derive(Debug, Clone, PartialEq, Eq)]
pub enum Subject {
    String {
        value: String,
        offset: usize,
        len: usize,
    },
    Pointer {
        value: PointerBuf,
        position: usize,
    },
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Report<E> {
    pub subject: Subject,
    pub error: E,
}

pub trait IntoReport: Sized {
    type Value;
    fn into_report(self, value: Self::Value) -> Report<Self>;
}

impl IntoReport for crate::ParseError {
    type Value = String;
    fn into_report(self, value: Self::Value) -> Report<Self> {
        let offset = self.complete_offset();
        let len = match &self {
            Self::NoLeadingBackslash => 1,
            Self::InvalidEncoding { offset, .. } => {
                if *offset < value.len() - 1 { 2 } else { 1 }
            }
        };
        Report {
            subject: Subject::String { value, offset, len },
            error: self,
        }
    }
}

The reason I'm mentioning it here is perhaps we could just return Report<ParseError> from PointerBuf::parse? It is a bit strange, naming wise, though.

Edit

remove Span and Location from IntoReport