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

Tweak docs and API for shared numeric types #217

Merged
merged 1 commit into from
Dec 24, 2021
Merged

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Dec 20, 2021

This is based off of #216, which should go in first.

Once we release 1.0, any public API that exists cannot be removed
without requiring us to release a new major version. Motivated by this,
I would to make sure that our public API surface is reasonable concise.

This PR removes a few helper methods and a few trait implementations. If
it turns out that these are needed in the future, they can be added back
without it being a breaking change.

@cmyr
Copy link
Member Author

cmyr commented Dec 20, 2021

I'm wondering if we could just get rid of IntegerOrFloat, and figure out a way we serialize as an integer when that is possible, at serialization time?

@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing 4d21066 against 0c7b43b

target old size new size difference
target/release/examples/load_save 1.9 MB 1.9 MB 192 Bytes (0.01%)
target/debug/examples/load_save 8.55 MB 8.55 MB -40 Bytes (-0.00%)

@cmyr
Copy link
Member Author

cmyr commented Dec 20, 2021

I also wonder if we could move these two types into font_info.rs, then they wouldn't need to be part of the top-level docs?

@madig
Copy link
Collaborator

madig commented Dec 21, 2021

I'm wondering if we could just get rid of IntegerOrFloat, and figure out a way we serialize as an integer when that is possible, at serialization time?

I'd be happy to not have a custom type for basic types just to get custom serialization behavior. I suppose we'd need a custom struct Kerning then which wraps a HashMap of stuff? It could even have a custom get(first, second) method for lookup, like defcon and ufoLib2 do :) I'd have done this before, but I bristled at boilerplate-reimplementing HashMap methods.

Base automatically changed from minor-reogranization to master December 21, 2021 16:36
Once we release 1.0, any public API that exists cannot be removed
without requiring us to release a new major version. Motivated by this,
I would to make sure that our public API surface is reasonable concise.

This PR removes a few helper methods and a few trait implementations. If
it turns out that these are needed in the future, they can be added back
without it being a breaking change.
@cmyr cmyr force-pushed the numeric-type-tweaks branch from b2e1269 to 3bdc179 Compare December 21, 2021 16:37
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing 13c3981 against df9f486

target old size new size difference
target/release/examples/load_save 1.9 MB 1.9 MB 192 Bytes (0.01%)
target/debug/examples/load_save 8.55 MB 8.55 MB -40 Bytes (-0.00%)

@cmyr
Copy link
Member Author

cmyr commented Dec 21, 2021

there's maybe one simple way? both IntegerOrFloat and NonNegativeIntegerOrFloat are only actually used in font_info, so we could just use a custom serializer/deserializer for specifically those fields, (#[serde(serialize_with = "integer_or_float")]) and that might be all we need? I'm going to look into this, but will do it as a separate PR.

Other than those questions, does this currently look okay to you @madig?

@madig
Copy link
Collaborator

madig commented Dec 21, 2021

That could work I think, actually. It loses the correct-by-construction thing but we have to call validate() anyway...

@cmyr
Copy link
Member Author

cmyr commented Dec 21, 2021

it would be correct by construction, though, since we will fail to deserialize if the number is negative?

@cmyr
Copy link
Member Author

cmyr commented Dec 21, 2021

oh I see your point. We could keep NonNegativeX around, if we wanted... 🤔

@madig
Copy link
Collaborator

madig commented Dec 21, 2021

Yes, maybe let's replace IntegerOrFloat by a serde attribute for now (FontInfo and Kerning I guess?) and tie the existence of the other to a better idea for how to implement FontInfo?

@cmyr
Copy link
Member Author

cmyr commented Dec 21, 2021

it's actually more annoying than I thought, because I can't get the attribute to work for the various Option<Vec<IntegerOrFloat>> 🤔

@madig
Copy link
Collaborator

madig commented Dec 21, 2021

Ah yes. I think you have to write serde functions for the whole type, not just the innermost part.

@cmyr
Copy link
Member Author

cmyr commented Dec 23, 2021

I couldn't figure out how to serialize/deserialize a sequence while also providing a custom serialization function for each member. I didn't dig too deeply, but I suspect I'll have to open an issue.

@cmyr cmyr merged commit 0e84611 into master Dec 24, 2021
@cmyr cmyr deleted the numeric-type-tweaks branch December 24, 2021 18:40
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.

2 participants