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

Fixup load api #141

Merged
merged 1 commit into from
Jun 29, 2021
Merged

Fixup load api #141

merged 1 commit into from
Jun 29, 2021

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Jun 28, 2021

This is mainly intended to clear up confusion around how to use
load_ufo; the answer is "don't".

I believe that the previous API of splitting up the creation of the font
from the loading was unnecessary. I also suspect that having the Font
object hold on to the DataRequest is unnecessary, although there are
possible arguments in favor of this approach.

In any case, the main confusion with the previous API is that loading
with a data request took &self by reference but returned a new font
object, which is confusing. I would expect it either to take a
DataRequest as an argument to an associated function, or I would expect
it to take &mut self and load the contents of the file into the
current font?

@ctrlcctrlv Does this change work for you? Do you ever do anything where you create a Font and then do some other stuff before calling load_ufo? My medium-term vision would be to maybe remove the data_request field on Font; alternatively we could keep it, but only exposed immutably (via a getter?) my thinking is that mutating the request after the font is loaded doesn't do anything, but could confuse the user?

closes #138

@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing ae135ea against 6f64b48

target old size new size difference
target/release/examples/open_ufo 1.69 MB 1.67 MB -18.74 KB (-1.08%)
target/debug/examples/open_ufo 7.05 MB 7.05 MB -1.84 KB (-0.03%)

This is mainly intended to clear up confusion around how to use
load_ufo; the answer is "don't".

I believe that the previous API of splitting up the creation of the font
from the loading was unnecessary. I also suspect that having the Font
object hold on to the DataRequest is unnecessary, although there are
possible arguments in favor of this approach.

In any case, the main confusion with the previous API is that loading
with a data request took `&self` by reference but returned a new font
object, which is confusing. I would expect it either to take a
DataRequest as an argument to an associated function, or I would expect
it to take `&mut self` and load the contents of the file into the
current font?
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing bfbbc1c against 07324c0

target old size new size difference
target/release/examples/open_ufo 1.69 MB 1.67 MB -18.7 KB (-1.08%)
target/debug/examples/open_ufo 7.05 MB 7.05 MB -1.92 KB (-0.03%)

@madig
Copy link
Collaborator

madig commented Jun 28, 2021

I don't think we need to keep request around, except when we eventually (?) implement lazy loading?

@madig
Copy link
Collaborator

madig commented Jun 29, 2021

@cmyr How bout making this

pub fn load(path: &dyn AsRef<Path>) -> Result<Font, Error> {
    Self::load_requested_data(path, DataRequest::default())
}

pub fn load_requested_data(
    path: &dyn AsRef<Path>,
    request: DataRequest,
) -> Result<Font, Error> {
    // ...
}

Or something so we don't have to have an inner impl function to aoid monomorphization?

@cmyr
Copy link
Member Author

cmyr commented Jun 29, 2021

Request was added in #53 because it was needed by @ctrlcctrlv, so I don't anticipate removing it.

I'd be happy to look at various patches to remove that inner function and see how they look?

@cmyr
Copy link
Member Author

cmyr commented Jun 29, 2021

I'm going to merge this as-is, and if fred has any comments we can always revise it later.

@cmyr cmyr merged commit 894db3a into master Jun 29, 2021
@cmyr cmyr deleted the fixup-load-api branch June 29, 2021 13:15
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.

Font::load <-> Font::load_ufo?
2 participants