Skip to content

docs: update description for brand typography "files" field #12268

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mcanouil
Copy link
Collaborator

@mcanouil mcanouil commented Mar 12, 2025

Clarify the expected schema for the files field in the brand typography section to specify that font files should be included as an array (e.g., - path: <path>).

This change addresses the issue where users encountered validation errors when using the source: file option in their _brand.yml files.

Partially help with #12267

This is more extensively documented in the brand.yml documentation, so I did not add more into the main "Quarto brand guide".
https://posit-dev.github.io/brand-yml/brand/typography.html#comprehensive-example-with-font-definitions

@mcanouil mcanouil self-assigned this Mar 12, 2025
@mcanouil mcanouil added the documentation Doc improvements & quarto-web label Mar 12, 2025
@mcanouil mcanouil changed the title docs: updatedescription for brand typography "files" field docs: update description for brand typography "files" field Mar 12, 2025
@mcanouil mcanouil requested a review from gordonwoodhull March 12, 2025 14:22
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment about this change as I don't think this is the best way to fix the problem reported.

Improving description is probably ok to do as this is simple for one field, but I don't think we should add in description how each fields need to be formatted. To me the schema is supposed to do that, and currently it does define it indeed with - <path> or path: <path>.

files:
arrayOf:
anyOf:
- path
- schema:
object:
properties:
path:
schema: path
description: >
The path to the font file. This can be a local path or a URL.
weight:
ref: brand-font-weight
style:
ref: brand-font-style

It feels like the error from #12267 are the problem. They do not help on how to fix, and this is where it should be explained. I can see this also in linting error which is not clear enough regarding the expected shema,
image

I am not sure how to fix as I am not yet expert in how the schema work. I just want to be sure to focus on the underlying problem (uninformative and unhelpful error).

We have some other fields with similar schema (like filters) and the message is (slightly) clearer
image

So it seems the wrong type of error is thrown from validation. 🤔

I think it would be good to fix that.

@mcanouil
Copy link
Collaborator Author

mcanouil commented Mar 12, 2025

I was not aware of the linting of the schema.
So, yes the error raised from it should be clearer.

I have added yaml-validation to the issue. This PR won't close the issue.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Mar 12, 2025

I agree with @cderv but I'm not sure how hard it would be to get a more specific/relevant error here.

IIUC the YAML validator would need to see that you are trying to use source: file and then emit a specific error having to do with not fitting the brand-font-file schema. Instead it is displaying an error a level up on brand-font

- id: brand-font
description: Font files and definitions for the brand.
anyOf:
- ref: brand-font-google
- ref: brand-font-bunny
- ref: brand-font-file
- ref: brand-font-system

and saying that you haven't matched brand-font-google. (I haven't looked at the YAML validator code.)

@cderv
Copy link
Collaborator

cderv commented Mar 12, 2025

I was not referring to that Google error - I agree this one is trickier.
I was referring to the files field one where passing a string only says property name files is invalid where I would expect something similar to passing a string to filters.

They kind of behave the same

filters:
  - process.lua

or

filters: 
  path: process.lua

This is the error that this PR tries to mitigate by adding information about how to format in description directly

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Mar 12, 2025

Got it.

I tried a few things but was not able to coax the validator to tell me that files should be an array, rather than telling me property name files is invalid, which makes it sound like files property name does not belong at all.

... even though the schema is very similar to pandoc-format-filters ...

IDK but I am guessing the error is better on filters because it is top-level.

@cderv
Copy link
Collaborator

cderv commented Mar 13, 2025

IDK but I am guessing the error is better on filters because it is top-level.

I came to the same conclusion too. We need to send the @cscheid signal !

@cscheid
Copy link
Collaborator

cscheid commented Mar 13, 2025

One of these days I really should walk us all through the implementation of the yaml validator so that you understand the source that's causing these error message bugs.

Yes, it's a bug. But before we start to fix it, I need to add support for testing against error messages in a way that we can include them in the test suite.

I don't want to change things to fix the issue here, and then make it worse for other errors...

@cderv
Copy link
Collaborator

cderv commented Mar 14, 2025

But before we start to fix it, I need to add support for testing against error messages in a way that we can include them in the test suite.

I need clarification on this. My understanding is that this should already be possible, at least for smoke tests, and maybe not for smoke-all document test.

I'm happy to tweak the tests suite to make what you have in mind happen. I made some improvements recently, so this is quite fresh to me.

@cscheid
Copy link
Collaborator

cscheid commented Mar 14, 2025

But before we start to fix it, I need to add support for testing against error messages in a way that we can include them in the test suite.

I need clarification on this. My understanding is that this should already be possible, at least for smoke tests, and maybe not for smoke-all document test.

Sorry, I wasn't clear. I'd like to have a mode where Quarto emits its validation errors in a structured format (JSON) so that our verifiers don't need to work on regular expressions. This will require changing the validator code itself, but it will be an important feature for the future anyway.

@cscheid cscheid modified the milestones: v1.7, v1.8 Apr 2, 2025
@mcanouil
Copy link
Collaborator Author

mcanouil commented Apr 7, 2025

Revert quarto build-js to avoid conflicts. It will be require to run it on or before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Doc improvements & quarto-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants