Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rejig error hierarchy #224

Merged
merged 15 commits into from
Jan 4, 2022
Merged

Rejig error hierarchy #224

merged 15 commits into from
Jan 4, 2022

Conversation

madig
Copy link
Collaborator

@madig madig commented Dec 28, 2021

This experimental PR implements a more layered error hierarchy for better visibility into the error chain.

Example, messing with the x attribute for a point:

Error: failed to load font 'testdata/MutatorSansLightWide.ufo'

Caused by:
    0: failed to load font
    1: failed to load layer 'foreground' from 'testdata/MutatorSansLightWide.ufo/glyphs'
    2: failed to load glyph 'C' from 'testdata/MutatorSansLightWide.ufo/glyphs/C_.glif'
    3: failed to parse glyph data
    4: bad number

It is not entirely possible to have the detailed errors be private, as we expose several public methods that now return them. This made me think: Should we expose e.g. FontInfo::from_file and the LayerSet::load and Layer::load methods as public?

TODO:

  • Decide visibility of new errors and therefore maybe limit the public API?
  • Rejig writing errors
  • Better errors for Glif parsing, GlifLoadError::Parse is a bit non-descript better done in a separate PR

@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 28, 2021
@linebender linebender deleted a comment from github-actions bot Dec 29, 2021
@chrissimpkins
Copy link
Collaborator

Looks fantastic Nikolaus 👍

@chrissimpkins
Copy link
Collaborator

I came across this in https://github.com/liborty/indxvec/blob/964de9f78a89844bd651f6709aff86f88751ff67/src/lib.rs#L5-L16 in case it happens to be helpful for the error handling revisions:

/// macro `here!()` gives `&str` with the current `file:line path::function` for error messages.
#[macro_export]
macro_rules! here {
    () => {{
        fn f() {}
        fn type_name_of<T>(_: T) -> &'static str {
            std::any::type_name::<T>()
        }
        let name = type_name_of(f); 
        format!("\n{}:{} {}", file!(), line!(), &name[..name.len()-3])
    }}
}

@madig
Copy link
Collaborator Author

madig commented Dec 29, 2021

Glyph loading errors are a bit finicky. There's dealing with XML, UFO spec compliance and builder logic 🤔

@madig
Copy link
Collaborator Author

madig commented Dec 29, 2021

LayerSetLoadError sits above LayerLoadError because LayerSet::load is currently public and I want to keep UfoLoadError private for now. Otherwise, I could use UfoLoadError instead I suppose and move the variants over.

@madig madig changed the base branch from master to simplify-load-object-libs December 30, 2021 15:17
@linebender linebender deleted a comment from github-actions bot Dec 30, 2021
@linebender linebender deleted a comment from github-actions bot Dec 30, 2021
@linebender linebender deleted a comment from github-actions bot Dec 30, 2021
Base automatically changed from simplify-load-object-libs to master January 1, 2022 20:06
@madig madig force-pushed the rejig-errors branch 2 times, most recently from c665502 to 3c64ad2 Compare January 4, 2022 19:17
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@madig
Copy link
Collaborator Author

madig commented Jan 4, 2022

Hm. I am probably falling into the traps described in https://www.unwoundstack.com/blog/rust-error-handling.html#org579e4a9. I'll try to reduce variants somewhat.

@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
Copy link
Member

@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.

One little nit, but I think this is an improvement!

src/error.rs Outdated Show resolved Hide resolved
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@linebender linebender deleted a comment from github-actions bot Jan 4, 2022
@github-actions
Copy link

github-actions bot commented Jan 4, 2022

🗜 Bloat check ⚖️

Comparing f922e9a against aca74c2

target old size new size difference
target/release/examples/load_save 1.9 MB 1.9 MB -1.13 KB (-0.06%)
target/debug/examples/load_save 8.57 MB 8.6 MB 22.25 KB (0.25%)

@chrissimpkins
Copy link
Collaborator

Possible to push a new release that includes the updated error handling that was merged over the last couple of months?

@madig
Copy link
Collaborator Author

madig commented Mar 3, 2022

Already done today 😁

@chrissimpkins
Copy link
Collaborator

Already done today 😁

The latest release and tag that I see is https://github.com/linebender/norad/releases/tag/v0.6.0 from Oct 2021?

@chrissimpkins
Copy link
Collaborator

It looks like crates.io was updated with a new release (v0.7.0). I checked the tags/releases here. Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants