Skip to content
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

Error tweaks #209

Merged
merged 11 commits into from
Dec 17, 2021
Merged

Error tweaks #209

merged 11 commits into from
Dec 17, 2021

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Nov 24, 2021

With this patch, we now track the path involved anytime we fail to load a .glif or a .plist file.

This also removes the position tracking for errors that occur when parsing glifs; there isn't a great way to get this information from quick_xml, and the information we did have was often inaccurate enough that it wasn't even on the correct line, which made it not very helpful for producing diagnostics.

@madig if you're interested in adding path information to other errors, I think you should be able to follow the pattern I've used here.

src/glyph/parse.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
@madig
Copy link
Collaborator

madig commented Nov 26, 2021

So the general pattern is, provide specific error types for some things and then wrap them with a path in the top-level Error type?

@madig
Copy link
Collaborator

madig commented Nov 26, 2021

With this branch, changing a content.plist file entry to intentionally point nowhere, I now get

Glif { path: "C:\\...\\Temp\\aaa\\MutatorSansLightWide.ufo\\glyphs\\E__.glif", inner: IoError(Os { code: 2, kind: NotFound, message: "The system cannot find the file specified." }

That's better. The path implicitly also includes the layer name, though it could be presented a bit nicer maybe 🤔

@madig
Copy link
Collaborator

madig commented Nov 26, 2021

Also, this makes the Error::ParseError variant redundant. Maybe I can even get rid of Error::IoError?

@linebender linebender deleted a comment from github-actions bot Nov 26, 2021
@cmyr
Copy link
Member Author

cmyr commented Nov 29, 2021

So the general pattern is, provide specific error types for some things and then wrap them with a path in the top-level Error type?

This is a function of the code's "separation of concerns": the glyph parsing code does not know the path, it just receives a pointer to some bytes. This means that whatever error it returns cannot include the path, so we have to add that information later.

If the glif parsing code did happen to know the path, this would be unnecessary, and I would include the path in the error there. I'm not advocating this, though, since the separation of concerns is good for any number of reasons: as a simple example it means that we can test glyph parsing without needing to worry about having the files written to disk.

cmyr and others added 4 commits November 29, 2021 12:37
This also removes the 'position' param from these errors, since it was
both not very helpful and not very accurate. It would be nice to be able
to do better error reporting of this stuff, but quick_xml doesn't really
expose enough info to be useful.
The read/write split might not be strictly necessary, but doesn't hurt.
Tracking the path feels like a good addition.
@cmyr
Copy link
Member Author

cmyr commented Nov 29, 2021

okay, fixed up and rebased :)

@linebender linebender deleted a comment from github-actions bot Dec 15, 2021
@linebender linebender deleted a comment from github-actions bot Dec 15, 2021
@linebender linebender deleted a comment from github-actions bot Dec 15, 2021
@linebender linebender deleted a comment from github-actions bot Dec 16, 2021
@linebender linebender deleted a comment from github-actions bot Dec 16, 2021
@linebender linebender deleted a comment from github-actions bot Dec 16, 2021
@linebender linebender deleted a comment from github-actions bot Dec 16, 2021
@linebender linebender deleted a comment from github-actions bot Dec 16, 2021
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing 1bb71e1 against ddaaaf6

target old size new size difference
target/release/examples/load_save 1.86 MB 1.88 MB 18.71 KB (0.98%)
target/debug/examples/load_save 8.43 MB 8.45 MB 27.28 KB (0.32%)

@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing ea988e4 against ddaaaf6

target old size new size difference
target/release/examples/load_save 1.86 MB 1.88 MB 18.75 KB (0.99%)
target/debug/examples/load_save 8.43 MB 8.45 MB 27.3 KB (0.32%)

Copy link
Member Author

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't approve this since I opened it, but I'm happy to merge. :)

@madig
Copy link
Collaborator

madig commented Dec 17, 2021

Approved it for you 😁

@cmyr cmyr merged commit e8ac638 into master Dec 17, 2021
@cmyr cmyr deleted the error-tweaks branch December 17, 2021 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants