Skip to content

Valid classifications and stroke#533

Merged
simoncozens merged 8 commits into
fonttools:mainfrom
emmamarichal:valid_classifications_and_stroke
Dec 17, 2025
Merged

Valid classifications and stroke#533
simoncozens merged 8 commits into
fonttools:mainfrom
emmamarichal:valid_classifications_and_stroke

Conversation

@emmamarichal
Copy link
Copy Markdown
Contributor

Description

At present, valid Classification values:
DISPLAY("Display"),
HANDWRITING("Handwriting"),
MONOSPACE("Monospace"),
SYMBOLS("Symbols");

Valid Stroke values:
SERIF("Serif"),
SLAB_SERIF("Slab Serif"),
SANS_SERIF("Sans Serif");

If a value is wrong, it blocks Sandbox server

Checklist

  • update CHANGELOG.md
  • wait for the tests to pass
  • request a review

@felipesanches
Copy link
Copy Markdown
Contributor

I think the check also needs to be registered (via .add_and_register_check) at fontspector/profile-googlefonts/src/lib.rs

@simoncozens
Copy link
Copy Markdown
Collaborator

Most of the checks on the validity of METADATA.pb live in googlefonts/metadata/validate.rs, and we're going down the route of having fewer checks which do more rather than lots of checks which each do little. So I would prefer to see this code incorporated in validate.rs - maybe ask your AI to do that? :-)

@emmamarichal
Copy link
Copy Markdown
Contributor Author

@felipesanches @simoncozens
I made the modifications! I integrated it as a separated check, is it the good way to proceed, or should I add it like:

    // Check stroke and classifications values
    const VALID_CLASSIFICATIONS: &[&str] = &["Display", "Handwriting", "Monospace", "Symbols"];
    const VALID_STROKES: &[&str] = &["Serif", "Slab Serif", "Sans Serif"];

    if let Some(stroke) = msg.stroke.as_ref() {
        if !stroke.is_empty() && !VALID_STROKES.contains(&stroke.as_str()) {
            problems.push(Status::fail(
                "invalid-stroke",
                &format!(
                    "METADATA.pb stroke field contains invalid value '{}'. Valid values are: {}",
                    stroke,
                    VALID_STROKES.join(", ")
                ),
            ));
        }
    }

    for classification in &msg.classifications {
        if !VALID_CLASSIFICATIONS.contains(&classification.as_str()) {
            problems.push(Status::fail(
                "invalid-classification",
                &format!(
                    "METADATA.pb classifications field contains invalid value '{}'. Valid values are: {}",
                    classification,
                    VALID_CLASSIFICATIONS.join(", ")
                ),
            ));
        }
    }

    return_result(problems)
}

@simoncozens simoncozens force-pushed the valid_classifications_and_stroke branch from cfcfb48 to 0acaab1 Compare December 17, 2025 11:31
@simoncozens simoncozens added this pull request to the merge queue Dec 17, 2025
Merged via the queue into fonttools:main with commit a7a2804 Dec 17, 2025
4 of 5 checks passed
simoncozens added a commit that referenced this pull request Dec 17, 2025
* add check for stroke and classifications in metadata.pb

* rustfmt

* remove valid_stroke_and_classifications.rs

* add the check in validate.rs

* removed mod valid_stroke_and_classifications; in mod.rs

* rustfmt

* update link (PR URL)

* chore(metadata/validate): Merge into existing validate check

---------

Co-authored-by: Simon Cozens <simon@simon-cozens.org>
@emmamarichal emmamarichal deleted the valid_classifications_and_stroke branch January 9, 2026 15:00
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.

3 participants