-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
updated schema #486
Draft
zkat
wants to merge
1
commit into
main
Choose a base branch
from
zkat/schema-v2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
updated schema #486
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
@kdl:schema "https://github.com/kdl-org/kdl/blob/main/schema/kdl-schema.kdl" | ||
|
||
metadata { | ||
// TODO: update this link when we're ready to release something. | ||
link "https://github.com/kdl-org/kdl/blob/main/schema/cargo.kdl" rel=self | ||
zkat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
title "Cargo Schema" lang=en | ||
zkat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description "KDL-based translation of the Cargo.toml schema." lang=en | ||
author "Kat Marchán" { | ||
link "https://github.com/zkat" rel=self | ||
} | ||
link "https://github.com/kdl-org/kdl" rel=documentation | ||
zkat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
link "https://doc.rust-lang.org/cargo/reference/manifest.html" rel=documentation | ||
license "Creative Commons Attribution-ShareAlike 4.0 International License" spdx=CC-BY-SA-4.0 { | ||
link "https://creativecommons.org/licenses/by-sa/4.0/" lang=en | ||
zkat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
children { | ||
node package title="Describes a package" { | ||
zkat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
children { | ||
node name title="The name of the package" { | ||
required | ||
zkat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
arg { | ||
type string | ||
pattern #"^[a-zA-Z0-0\-_]+$"# | ||
} | ||
} | ||
node version title="The version of the package." { | ||
arg { | ||
type string | ||
// From https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string | ||
pattern #"^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$"# | ||
} | ||
} | ||
node authors title="The authors of the package." { | ||
repeatable | ||
args { | ||
distinct | ||
type string | ||
} | ||
children { | ||
node - { | ||
repeatable | ||
arg title="Name" { | ||
type string | ||
} | ||
prop email title="Email address" { | ||
type string | ||
format email | ||
} | ||
prop about title="Brief note about author (role, etc)" { | ||
type string | ||
} | ||
} | ||
} | ||
} | ||
node edition title="The Rust edition." { | ||
arg { | ||
type string | ||
enum "2015" "2018" "2021" "2024" | ||
} | ||
} | ||
node rust-version title="The minimal supported Rust version." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node description title="A description of the package." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node documentation title="URL of the package documentation." { | ||
arg { | ||
type string | ||
format url | ||
} | ||
} | ||
node readme title="Path to the package’s README file." { | ||
arg { | ||
type string #boolean | ||
} | ||
} | ||
node homepage title="URL of the package homepage." { | ||
arg { | ||
type string | ||
format url | ||
} | ||
} | ||
node repository title="URL of the package source repository." { | ||
arg { | ||
type string | ||
format url | ||
} | ||
} | ||
node license title="The package license." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node license-file title="Path to the text of the license." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node keywords title="Keywords for the package." { | ||
args { | ||
type string | ||
// No pattern because keyword restrictions are only on | ||
// crates.io | ||
} | ||
} | ||
node categories title="Categories of the package." { | ||
args { | ||
type string | ||
// No pattern because category restrictions are only on | ||
// crates.io | ||
} | ||
} | ||
node workspace title="Path to the workspace for the package." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node build title="Path to the package build script." { | ||
arg { | ||
type string boolean | ||
} | ||
} | ||
node links title="Name of the native library the package links with." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node exclude title="Files to exclude when publishing." { | ||
args { | ||
type string | ||
} | ||
} | ||
node include title="Files to include when publishing." { | ||
args { | ||
type string | ||
} | ||
} | ||
node publish title="Can be used to prevent publishing the package." { | ||
// TODO: This is a good example of where we might need smarter | ||
// comstraints ("either a single boolean, or 1+ strings") | ||
args { | ||
type string boolean | ||
} | ||
] | ||
node metadata title="Extra settings for external tools." { | ||
repeat | ||
args | ||
props { | ||
allow-others | ||
} | ||
} | ||
node default-run title="The default binary to run by cargo run." { | ||
arg { | ||
type string | ||
} | ||
} | ||
node no-autolib title="Disables library auto discovery." | ||
node no-autobins title="Disables binary auto discovery." | ||
node no-autoexamples title="Disables example auto discovery." | ||
node no-autotests title="Disables test auto discovery." | ||
node no-autobenches title="Disables bench auto discovery." | ||
node resolver title="Sets the dependency resolver to use." | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Where will this
@kdl:schema
be used? If it doesn't have to be a reachable URL, then do you still think that it's valuable to have as a top-level key like this?Do you envision KDL documents adding this to their top-level? Does that mean users are expected to handle this node when parsing with
kdl-rs
? Or should KDL parsers remove this node from the AST before passing it on to user code? If so, then I think that would require a change to the KDL spec?I'm not opposed to this, though I'd personally probably keep them separate and give any sort of validator two files (the data and the schema it follows) — unless I'm misunderstanding though, I think this might be better as a field in
metadata
.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.
This is similar to JSONSchema's
$schema
, which points to a valid (relative) URL.This is not required, e.g. I can edit my
angular.json
and get LSP support because the Angular VSCode plugin interprets any file calledangular.json
as an Angular configuration document.But, for schemas that don't come with editor plugins or that don't have a filename convention, it is very handy that you can add that
$schema
property and the JSON plugin will kick in and provide LSP support.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.
Okay, I can definitely see that as nice to have for editor / tooling support! Would implementations like
kdl-rs
strip this out of the AST returned to users? Essentially just because I agree it would be something nice to add to get LSP support, but I wouldn't want that addition to affect any result of parsing!Is this the sort of thing that could / should be a standard-formatted comment? Like how sometimes people seem to set vi / vim settings in comments with
vi:
or whatever at the top of a file?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.
Users would have to ignore this field, if they want it to be present. Just like folks are free to ignore
$schema
when using JSONSchema.I thought about using a comment for this, but that seems... somehow worse? I don't know. I feel like it should just be data.
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.
That's fair... I'll sleep on it — I'd personally find it a little wacky to have to describe in my schema for a KDL format the node that will be used to refer back to that schema. So, if I wanted to do what @bgotink mentioned, and link my current KDL file to a schema for editor support, I'd need to make sure that that schema includes the
@kdl:schema
node in it as well? If it's even a URL linked to a file I can edit?But, before I whinge any more, I'll read up properly on
$schema
in JSON and do some proper thinking!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.
I've changed this so you no longer need to remember to include this in your own schemas: it is now a default item for all schemas, and can be disabled by doing
node @kdl:schema { undefine }
.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.
There are multiple versions of JSONSchema. The
$schema
in the schema is used to define the version of JSONSchema in which it is written, so it serves an extra purpose on top of enabling your editor to provide support while writing schemas.For example, data file "my-data.kdl" has
@kdl:schema "./my-schema.kdl"
, schema filemy-schema.kdl
would then have@kdl:schema "https://kdl.dev/schema/v0.2.kdl";
(fictitious URL)Agreed, both because it feels bad and because it's actually useful data. Just like the version of JSONSchema, this could be used to differentiate between different versions of configuration file formats. If this were a comment, you'd have to have a comment to a versioned URL ánd a version indicator in the data.
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.
Okay, I've done some pondering and perhaps come to the conclusion that the first draft of this was best, but with some caveats. If I'm understanding correctly, it sounds like there's two, slightly distinct, reasons to want something like this:
But one of my problems remains if the
@kdl:schema
must be an uncommented node: say I'm working on a certain type of KDL config file — it's the type of thing that's complex enough that I'd like my LSP to know the schema and validate my edits. The issue, however, is that this config file is read by some code that looks like this:Where the important part is the
#[serde(deny_unknown_fields)]
— which is think is a nice thing to have if you want to enforce that config files are free of unrecognized sections, something that could help a lot in catching user typos. Now I'm in a bit of a bind because, once again, I have to switch between 1) having@kdl:schema
uncommented, so LSP support works but the config can't actually be loaded, or 2) commenting out that node, losing LSP support, but actually being able to test the config with the program it's meant for.Obviously, however, this is a non-problem if the program already uses
@kdl:schema
as a standard convention for specifying a version:Then leaving
@kdl:schema
uncommented 100% of the time is perfectly fine!To address both of these cases, we could:
@kdl:schema
is the idiomatic way to version KDL formats, and set a good example by having thekdl-schema.kdl
define the optional@kdl:schema
node@kdl:schema
node, I suppose anywhere in the file, or a/- @kdl:schema
comment at the top of the file for when you want editor support, but the KDL format you're writing disallows it.@kdl:schema
to keep track of versions, then I think it should say so explicitly.In summary then, I think rolling this back to how it started (schemas must explicitly allow
@kdl:schema
) and making it clear that language servers should recognize both@kdl:schema
and/- @kdl:schema
(maybe allow other commented forms?) would be my ideal solution.That way even if someone's program is explicitly disallowing unknown fields (to save users from typos) and doesn't care to version files, I can still get editor support.
Let me know what you think about demystifying the
@kdl:schema
node and "downgrading" it to something that's idiomatic convention / recognized by tooling.SIDENOTE: Serde support for KDL will need to make decently heavy use of
#[serde(rename = "...")]
, andquick-xml
, for example, already uses the "@..." prefix for attributes. I was thinking perhaps we could use "$..." for properties (since they felt kind shell-variable-ish), but we'll likely need at least one other special prefix reserved. If we used@
for that, like#[serde(rename="@arguments")]
or whatever, then reading@kdl:schema
would become a bit more awkward (though not impossible, I don't think?). Might be a bit bike-sheddy, but we should perhaps think about either removing the@
, or rule it out as a potentialserde
prefix!Sorry for the flip-flop on this, but I think this is a solution I'd be 100% happy with <3
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.
Thinking through it, the main difference here with JSONSchema is that it allows other properties by default. That is,
"$schema": "foo"
won't fail validation.It would be a weaker validation, but we could always just default to allowing all children/props/args, and replace
allow-others
withdisallow-others
. Basically, make things more permissive by default, but let people constrain stuff. We can also add anallow-ksl
to specifically allow KSL-related keywords.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.
I'll have more time to reply to other comments soon, but for this one, I do like KSL being much more stringent (disallowing others by default), but I would be okay with something like
allow-ksl
if you think it would be useful — though maybe that only makes sense if there are several KSL keywords eventually?Unfortunately, I don't think that would solve the issue of an application not using KSL, just using
serde
and me wanting to add that LSP-type support (assuming I can't change theserde
derive myself). Because in that case there is no schema validation being done (via KSL, anyways). For what it's worth,knus
/knuffle
by default disallow unknown nodes, so I couldn't get schema-enhanced editor support in any of my current projects unless I could have/-@ksl:schema
commented.But I'm happy with what you originally had + letting the LSP pick up the node in comment form as well? If the user wants to use
@ksl:schema
as a real node / version, then I do think they should be required to specify that in their schema — like you originally had it :)That's my personal preference, anyways — keeping things very explicit and minimizing special cases :)