Skip to content
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

Allow to validate yaml file against its modeline schema #526

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nikaro
Copy link

@nikaro nikaro commented Feb 14, 2025

Hello 👋

This is a quick and dirty attempt at implementing #340

I submit it to gather some feedback before eventually going further... Am i going in the right way? What would your suggestions and improvements?

Regards.

Comment on lines +116 to +120
@click.option(
"--modeline-schema",
is_flag=True,
help="Use the schema defined in the modeline.",
)
Copy link
Author

Choose a reason for hiding this comment

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

Maybe --yaml-modeline-schema? Since this is very specific to YAML for now... or maybe going with this and raising a NotImplementedError on other content than yaml?

pattern = r"^# yaml-language-server: \$schema=(?P<schema>.*)$"
match = re.match(pattern, modeline.decode())
if not match:
raise Exception("Modeline with schema not found.")
Copy link
Author

Choose a reason for hiding this comment

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

Maybe there is something better than raising a bare Exception here.

@sirosen
Copy link
Member

sirosen commented Feb 17, 2025

Thanks for the idea and the PR!

I'm still thinking about this, but I'm actually pretty well inclined towards this solution. I've wanted a generic flag for picking up on the schema from files for YAML modelines and from JSON files which use $schema to specify their schemas, but I think doing YAML modeline support and limiting it to a single file is a very reasonable starting point.

My big questions right now are about how we parse the modeline. And we'll need some tests to validate the behavior.
Is the modeline required to be the first line in the file? I would expect that it could show up several lines later. And I think maybe it should be supported if it's indented or has irregular whitespace, so there's probably some room for testing and refinement of the parsing.

I need to give this more attention to get the details right, but this is a great start!

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.

2 participants