Refactor parsing in kicad_rs#11
Conversation
luxas
left a comment
luxas
left a comment
There was a problem hiding this comment.
Thanks for the refactor in the first two commits. Can we merge now and I'll rebase my work on top of this as well?
|
|
||
| use crate::types::*; | ||
|
|
||
| type ParseResult<T> = Result<T, Box<dyn Error>>; |
There was a problem hiding this comment.
If we make an utils file, this would make a pretty good DynamicError type
There was a problem hiding this comment.
Or DynamicResult, for now it can live here, but we should definitely do that move if it's also used elsewhere.
There was a problem hiding this comment.
DynamicResult Oh, yeah that was what I meant 😂
| let kisch = kicad_parse_gen::read_schematic(path)?; | ||
|
|
||
| // Parse the fields for the schematic | ||
| let meta = parse_meta(&kisch, path)?; |
There was a problem hiding this comment.
None if these 4 seem to actually require the "whole" of a kicad_parse_gen::Schematic. Maybe you can scope this down to just the description, components vector, etc.?
There was a problem hiding this comment.
Since it's passed as an immutable reference, I don't see the need to restrict the input scope unnecessarily if we decide to add/change the functionality of those functions later to also look at other fields (for example having parse_meta count the components, which is not possible with just with kisch.description).
There was a problem hiding this comment.
Ok, I have no strong preference. It's good as-is
I've changed the PR message and title now, so you can go ahead and merge if it looks good to you. |
2 participants
Continuation of #8 now that the common types and routines have been integrated from the
parserside. Makes some necessary changes to help facilitate code sharing for theevaluator.WIP, cc @luxas @chiplet