typst / ecow

Compact, clone-on-write vector and string.
Apache License 2.0
205 stars 16 forks source link

Public API structure #18

Closed CosmicHorrorDev closed 1 year ago

CosmicHorrorDev commented 1 year ago

I think it would make sense to expose the inline string size limit and then have the public API be something along the lines of

ecow::{
    eco_vec,
    format_eco, // Maybe rename to eco_format to keep name ordering consistent?
    vec::{EcoVec, IntoIter},
    str::{EcoString, INLINE_SIZE_LIMIT},
}

The existence of IntoIter to me is enough to have a separate vec module at least (and there is the chance that other iterators could be added like Drain)

I would be up for re-exporting or exclusively exposing EcoVec and EcoString in the root of the API though since they are likely going to be the most commonly use types

laurmaedje commented 1 year ago

I can see how this would make sense. I would probably rename the str module to string because it's an owned string. I would also like to see the re-exports because it's just too inconvenient otherwise.

ecow::{
    eco_vec,
    eco_format,
    vec::{EcoVec, IntoIter},
    string::EcoString,
    /* re-exports for EcoVec and EcoString */
}

What I don't really see the value in is exporting the inline size limit. What would be the use case for accessing that constant?

laurmaedje commented 1 year ago

While we're at restructuring: If there's a tests dir, then the src/tests.rs contents should move there too, I guess. These currently use the LIMIT constant (which I'd prefer to keep private), but I think just hardcoding 15 and adding a few extra test around 23 for big-endian is fine.

CosmicHorrorDev commented 1 year ago

What I don't really see the value in is exporting the inline size limit.

I think it's a good idea just because it makes the max inline size transparent. I would understand making it public+semver-exempt, but people may be relying on the inline limit being at least some size while currently it can change without much of a way to inspect it


:+1: to everything else

laurmaedje commented 1 year ago

I would be okay with an associated const on EcoString instead of a top-level const. Also, I think that INLINE_SIZE_LIMIT is a bit verbose and long, I think LIMIT is clear enough.

CosmicHorrorDev commented 1 year ago

Care to meet in the middle with something like INLINE_LIMIT? IMO public consts should have self-descriptive names while just LIMIT is a bit vague as to what it's referring to

laurmaedje commented 1 year ago

You're driving a hard bargain. Alright, then.