ref: Move sourcelinks to symbolic-common #803
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #803 +/- ##
==========================================
- Coverage 75.03% 74.80% -0.23%
==========================================
Files 71 63 -8
Lines 15630 15603 -27
==========================================
- Hits 11728 11672 -56
- Misses 3902 3931 +29 |
symbolic-common/src/sourcelinks.rs
Outdated
| /// Parse a `SourceLinkMapping` struct from a list of `documents` json values. | ||
| /// | ||
| /// See [Source Link PPDB docs](https://github.com/dotnet/designs/blob/main/accepted/2020/diagnostics/source-link.md#source-link-json-schema). | ||
| pub fn parse_from_documents(jsons: &[&[u8]]) -> Result<Self, serde_json::Error> { | ||
| #[derive(Deserialize)] | ||
| struct Documents { | ||
| documents: BTreeMap<Pattern, String>, | ||
| } | ||
|
|
||
| let mut mappings = BTreeMap::new(); | ||
|
|
||
| for &json in jsons { | ||
| let docs: Documents = serde_json::from_slice(json)?; | ||
| mappings.extend(docs.documents.into_iter()); | ||
| } | ||
|
|
||
| Ok(Self { mappings }) | ||
| } |
There was a problem hiding this comment.
Hm, instead of having a Deserialize and depending directly on serde, I would rather have a new(iter: impl Iterator<Item=(&str, &str)>) which consumes just pattern, value pairs. No serde involved at that point.
It would then consume the iterator and parse/build its Pattern types, sort that list and store it. The custom Ord impl is a bit overkill compared to the previous sorting code.
The serde impl is also a bit overkill compared to a simple "consume key/value pairs" code. But I do see your point that this would basically give you "parse sourcebundle manifest" functionality for free.
Though I would really want to keep this shared code as simple and possible, and also move specialized parse_from_documents to the ppdb crate, as that is very specific to ppdb.
There was a problem hiding this comment.
I agree that the parse_from_documents stuff is better in symbolic-ppdb. For the rest of the implementation, would you rather have the old version? Shouldn't be too hard to implement De/Serialize for that one either.
loewenheim
changed the title
|
The implementation has been simplified and the |
Co-authored-by: Arpad Borsos <[email protected]>
In preparation of using sourcelinks in sourcebundles, this extracts the sourcelink functionality from
symbolic-ppdband moves it tosymbolic-common. It also refactors the implementation based onserde.The JSON representation of a
SourceLinkMappingsstruct looks like this:One downside of this is that it makes the
serdedependency ofsymbolic-commonnon-optional. I don't know how big a deal this really is, but it may be that this code might more appropriately live in a different place.