-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: Add support for range mappings proposal #77
Changes from all commits
327c2af
879fb1b
9de6692
b13e00c
7558335
502e9a9
9292815
0cc6b20
cecc1bf
88a054c
9631444
4935322
24cda13
26eeeed
3d43ddf
f517b36
e4ad541
a7f241e
46d337d
65ba538
5cd2046
4535a2b
fc2ee72
1a1dbc5
af19537
be20676
3b72696
59b3241
0834058
26c6280
3f80f13
0be3f85
a8d6470
a040b52
8383467
6f5a4ae
3cf9dd6
bc2a237
38bb516
2ab0d3f
37cdbc7
3150d83
38785c3
09b72f3
b0d9e1d
f4867b1
46d64fa
f70a754
a025248
92ce26d
af22bf6
f80aa98
f8d1edd
544605d
e53ed5f
ca6d0c5
b6fd204
d8e5bbe
2fedd9f
8538bd6
a7f3356
95ff8b8
b5c4ca8
df6cc81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
kdy1 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,10 @@ pub enum Error { | |
InvalidRamBundleEntry, | ||
/// Tried to operate on a non RAM bundle file | ||
NotARamBundle, | ||
/// Range mapping index is invalid | ||
InvalidRangeMappingIndex(data_encoding::DecodeError), | ||
Comment on lines
+48
to
+49
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. Just a NOTE: this is a breaking change as 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 would say if we're ok with making a breaking change here, then we should also put the information whether a token is a range token directly on |
||
|
||
InvalidBase64(char), | ||
} | ||
|
||
impl From<io::Error> for Error { | ||
|
@@ -78,6 +82,12 @@ impl From<serde_json::Error> for Error { | |
} | ||
} | ||
|
||
impl From<data_encoding::DecodeError> for Error { | ||
fn from(err: data_encoding::DecodeError) -> Error { | ||
Error::InvalidRangeMappingIndex(err) | ||
} | ||
} | ||
|
||
impl error::Error for Error { | ||
fn cause(&self) -> Option<&dyn error::Error> { | ||
match *self { | ||
|
@@ -114,6 +124,8 @@ impl fmt::Display for Error { | |
Error::InvalidRamBundleIndex => write!(f, "invalid module index in ram bundle"), | ||
Error::InvalidRamBundleEntry => write!(f, "invalid ram bundle module entry"), | ||
Error::NotARamBundle => write!(f, "not a ram bundle"), | ||
Error::InvalidRangeMappingIndex(err) => write!(f, "invalid range mapping index: {err}"), | ||
Error::InvalidBase64(c) => write!(f, "invalid base64 character: {}", c), | ||
} | ||
} | ||
} |
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.
TBH, clippy is absolutely right to complain here. Trying to review the tests for this on github, I completely lose track of what all these parameters mean (without inlay hints).
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.
Yeah, but I'm not sure how should I fix it.
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.
Yes, me neither :-( I think this is fine this way, just wanted to call it out as a future improvement opportunity.
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.
Yeah this is definitely on the list for an eventual rework. These signatures can't stay like this.