-
Notifications
You must be signed in to change notification settings - Fork 351
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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>
.
quarto-cli/src/resources/schema/definitions.yml
Lines 2923 to 2937 in 2ae62ed
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,
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
So it seems the wrong type of error is thrown from validation. 🤔
I think it would be good to fix that.
I was not aware of the linting of the schema. I have added |
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 quarto-cli/src/resources/schema/definitions.yml Lines 2811 to 2817 in 2ae62ed
and saying that you haven't matched |
I was not referring to that Google error - I agree this one is trickier. They kind of behave the same
or
This is the error that this PR tries to mitigate by adding information about how to format in description directly |
Got it. I tried a few things but was not able to coax the validator to tell me that ... even though the schema is very similar to IDK but I am guessing the error is better on |
I came to the same conclusion too. We need to send the @cscheid signal ! |
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... |
I need clarification on this. My understanding is that this should already be possible, at least for smoke tests, and maybe not for 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. |
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. |
Revert |
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