Skip to content

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

Merged
cderv merged 3 commits into
quarto-dev:mainfrom
mcanouil:fix/issue12267
Oct 26, 2026
Merged

cderv merged 3 commits into
quarto-dev:mainfrom
mcanouil:fix/issue12267

Conversation

@mcanouil

@mcanouil mcanouil commented Mar 12, 2025 •

Copy link
Copy Markdown
Collaborator

Description

The files field under brand typography now starts its description with "A list of font files to include."
That hint shows in editor completion and hover.
If someone passes a single string, the validator says property name files is invalid, and the hint tells them files takes a list.
The item shapes stay in the schema and are not repeated in the text.

The build artifacts are regenerated, so vs-code.mjs and the YAML intelligence resources ship the new text.
Each generated file changes only in the files description.

Related to #12267.
This does not fix the validator error message itself.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes
  • updated the appropriate changelog in the PR
  • ensured the present test suite passes
  • added new tests
  • created a separate documentation PR in Quarto's website repo and linked it to this PR
AI-assisted PR
  • AI tool used: Claude Code.
  • Codebase grounding: local clone.
  • Human review: I have reviewed, tested, and verified the AI-generated content before submitting.

@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
Comment thread src/resources/schema/definitions.yml Outdated
@mcanouil
mcanouil requested a review from gordonwoodhull March 12, 2025 14:22

@cderv cderv left a comment

Copy link
Copy Markdown
Member

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

mcanouil commented Mar 12, 2025 •

Copy link
Copy Markdown
Collaborator Author

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

gordonwoodhull commented Mar 12, 2025 •

Copy link
Copy Markdown
Member

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

cderv commented Mar 12, 2025 •

Copy link
Copy Markdown
Member

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

gordonwoodhull commented Mar 12, 2025 •

Copy link
Copy Markdown
Member

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

cderv commented Mar 13, 2025

Copy link
Copy Markdown
Member

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

cscheid commented Mar 13, 2025

Copy link
Copy Markdown
Member

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

cderv commented Mar 14, 2025

Copy link
Copy Markdown
Member

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

cscheid commented Mar 14, 2025

Copy link
Copy Markdown
Member

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

mcanouil commented Apr 7, 2025

Copy link
Copy Markdown
Collaborator Author

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

@cscheid cscheid modified the milestones: v1.8, v1.9 Aug 20, 2025
@mcanouil
mcanouil marked this pull request as draft January 14, 2026 22:34
@mcanouil

Copy link
Copy Markdown
Collaborator Author

Should I close and let you take over?

@gordonwoodhull

gordonwoodhull commented Jan 15, 2026 •

Copy link
Copy Markdown
Member

I don’t think we have a clear plan for improving validation until 2.0.

I continue to get bit by the underlying issue (esp with brand because it’s deeply nested) but I just tell myself that the errors don’t say quite what they mean; in this case it sounds like the key files is invalid, but it is the contents of files that is invalid.

I’m inclined to take the patch but make the description shorter so it’s a hint and not duplicating the schema.

I’ll take this.

@gordonwoodhull gordonwoodhull self-assigned this Jan 15, 2026
@cscheid cscheid modified the milestones: v1.9, v1.10 Feb 17, 2026
@mcanouil mcanouil modified the milestones: v1.10, v1.11 Jul 30, 2026
Say that files is a list, and leave the item shapes to the schema. Regenerate the build artifacts so editor tooling ships the new text.
@mcanouil

Copy link
Copy Markdown
Collaborator Author

@gordonwoodhull I've rebased this on main and cut the description down to a hint, as you suggested: "A list of font files to include."
The build artifacts are regenerated, so this is ready whenever you want to take it.

@mcanouil
mcanouil marked this pull request as ready for review October 25, 2026 20:36
@posit-snyk-bot

posit-snyk-bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@cderv cderv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the rebase. The short hint is what we discussed, it does not duplicate the schema anymore.

I regenerated the build artifacts on this branch and they match what you committed, so nothing else to rebuild before merge. LGTM.

@cderv
cderv merged commit 338be04 into quarto-dev:main Oct 26, 2026
52 checks passed
@mcanouil
mcanouil deleted the fix/issue12267 branch October 26, 2026 11:26
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.

5 participants