Repository navigation
fix: escape Unicode and reserved characters in Gerber serialization - #52
Conversation
|
|
||
| /// Format a Gerber string or field as printable ASCII with Unicode escapes. | ||
| /// Names and macro expressions have their own syntax and must not use this helper. | ||
| pub(crate) struct Escaped<'a>(pub(crate) &'a str, pub(crate) bool); |
There was a problem hiding this comment.
from the code in this PR, it's entirely unclear to me why the bool argument exists.
- there are no comments by it's definition indicating what it might do.
- all the code that uses it has
trueorfalse, there are no named constants anywhere that give a hint.
Without understanding it, it feels like there are two potention paths, assuming false means unescaped/raw:
- rename Escaped to MaybeEscaped with two enum values,
EscapedandRaw, eliminating the boolean. - change the
boolargument to be an enum value with two optionsEscapedandRaw, but this won't read well where it's used. - something else, perhaps using
Into<...>instead if Escaped and provide conversion methods as appropriate?
Not sure, but one thing I do know from scanning this PR, is that bool doesn't seem to be the right answer.
There was a problem hiding this comment.
The bool defines whatever it is type string or field, the diffrence here is in commas should be escaped or not
Would it be better if weturn the thing into an enum like EscapedGerberText::String(text) and EscapedGerberText::Field(text)?
There was a problem hiding this comment.
I pushed some changes, should be more clear now...
There was a problem hiding this comment.
so much better now! thanks for sticking with me on this.
No description provided.