ameba icon indicating copy to clipboard operation
ameba copied to clipboard

Add a JSON Schema for `.ameba.yml` and a CLI tool for automatically generating it

Open nobodywasishere opened this issue 1 year ago • 4 comments

When added to JSON Schema Store, this will allow autocomplete and documentation information for the .ameba.yml format. Instead of writing one by-hand like was the case for shard.yml, instead I wrote a CLI tool that will automatically generate it and update it as new rules are added and changed.

https://github.com/user-attachments/assets/0e507ba0-e01a-4e04-b185-50945ed7f9f0

nobodywasishere avatar Nov 17 '24 14:11 nobodywasishere

That looks great, thanks! ❤️ I'll review it sometime in the upcoming days.

Sija avatar Nov 17 '24 19:11 Sija

How feasible would it be to make running this script / updating the schema a part of CI? That way ppl don't have to remember to run it for it to stay up to date.

nobodywasishere avatar Nov 17 '24 20:11 nobodywasishere

@nobodywasishere That certainly makes sense. It needs to be automated in order to make it hassle-free for the end-users.

Sija avatar Nov 17 '24 20:11 Sija

That certainly makes sense. It needs to be automated in order to make it hassle-free for the end-users.

Didn't feel right about having a CI do commits, so instead added one that will fail if the user needs to run shards run schema, lmk if this works.

nobodywasishere avatar Nov 26 '24 05:11 nobodywasishere

Didn't feel right about having a CI do commits, so instead added one that will fail if the user needs to run shards run schema, lmk if this works.

I'm gonna insist on having this automated (I see nothing bad in CI doing commits, btw) :) The end user IMO shouldn't be concerned with a need to operate tooling like this - except for the case when we're talking about rules development, then it makes sense, yet still more for local development than for our main repo.

Sija avatar Oct 18 '25 00:10 Sija

@Sija IMO that requirement is asking too much at once. It brings a completely new concept into the CI game, which isn't even directly related. This feels like a very unnecessary obstacle.

This PR is a complete feature. It makes sure the schema cannot go out of sync. That's a solid base and should be acceptable.

Automating the synchronization is an enhancement for convenience. It should not be a hard requirement. Everything is safe and sound without it. It just replaces a round of potential manual intervention. But it's not a complex process to run the schema generator and commit its changes if CI tells you it needs an update.

Currently, this repo has not a single CI job that performs any automatic commits. So it seems very reasonable not to ask for introducing a completely new CI functionality on top of a feature change.

straight-shoota avatar Oct 18 '25 09:10 straight-shoota

(I see nothing bad in CI doing commits, btw) :)

Maybe it could be fine, but I see it potentially adding to noise if someone's working on a rule, the commit being added and removed, etc, versus if it's just a simple command the dev needs to run once the rule is done being developed to pass CI.

The end user IMO shouldn't be concerned with a need to operate tooling like this

The end user won't need to run the schema command, it's only meant for developers. The committed version of the schema file on master is all people need to worry about (or the schema file for their version of ameba - if people want to get that granular).

nobodywasishere avatar Oct 23 '25 04:10 nobodywasishere

Looks great! One thing, though, that still needs some attention is the size of the resulting json file. I believe it could be cut down by extracting the repetitive parts (common rule properties + severity enum) into their own (sub)schemas.

Sija avatar Nov 11 '25 02:11 Sija

Good point! Quick and easy de-dups brings it from 3900 to 2100 lines

nobodywasishere avatar Nov 11 '25 03:11 nobodywasishere

Needs definitions update and it's GTG 🚀

Sija avatar Nov 11 '25 20:11 Sija

@straight-shoota Sorry for the late reply! I had to sleep over the topic, and I agree that's too much to ask in such a case. I plan to integrate the schema generator into the CI automation anyway, since I still believe it's the way to go. This might be done in a separate PR, though, so there's no need to conflate these two things. Thanks for bearing with me! 🙏 🐻

Sija avatar Nov 11 '25 20:11 Sija

I think it may be good to move away from force pushes, makes PRs a lot less clear, and we can just squash when merging to master

nobodywasishere avatar Nov 11 '25 20:11 nobodywasishere

@nobodywasishere Yeah, I've been kinda fuzzy on this, since in general I prefer having atomic changes per commit, as I find it easy to browse through the history this way l8r on. In some cases, if the PR commit history is messy, I'd prefer squashing it before merging to avoid noise. As you can see, there's a bit of inner-conflict in terms of using single approach for all of the cases 😅

Sija avatar Nov 11 '25 20:11 Sija

Next step would be adding it to the Schema Store, is that correct?

Sija avatar Nov 11 '25 21:11 Sija

Yes, just need to create a PR there

nobodywasishere avatar Nov 11 '25 21:11 nobodywasishere

Looking at the Schema Store spec, it seems that there need to be a schema generated for the Ameba v1.6.4 (I reckon by applying the patch from this PR over the v1.6.4 tag) and only this published, since there's no formal v1.7.0 yet. Hmm, shit - but then, there still would be no file to link to, as v.1.6.4 is already tagged...

Looking at this more closely, I think we might want to postpone this until v1.7.0 release...

Sija avatar Nov 11 '25 21:11 Sija

I don't think that's necessary. We can just have the schema store point to whatever the latest schema is on master. I also don't think these are necessarily versioned.

This should be all that's necessary: https://github.com/SchemaStore/schemastore/pull/3669

nobodywasishere avatar Nov 11 '25 21:11 nobodywasishere

Then, the suggestions might end up incorrect due to the possible version difference between master and the locally installed version.

Sija avatar Nov 11 '25 21:11 Sija

That's completely fine imo. We don't know what version of ameba they're using and master provides the most up-to-date version. More accurate autocompletes n such can be provided by ameba-ls

nobodywasishere avatar Nov 11 '25 21:11 nobodywasishere

We don't know what version of ameba they're using [...]

Do we? That's what piqued my interest, since I've seen versioned schemas in there 🤔

Sija avatar Nov 11 '25 21:11 Sija

I haven't seen any if you can link to one

Do we?

I meant client-side, in vscode or w/e. I don't know how vscode would know which schema to select beyond the most recent, as it doesn't involve ameba binaries at all.

In terms of releasing a v1.6.4 schema, we can just create a new v1.6.4-schema tag w/ this commit and point to that in the schema store. Doesn't need to be in the v1.6.4 tag

nobodywasishere avatar Nov 11 '25 22:11 nobodywasishere

Me forgetting that the Version prop exists

nobodywasishere avatar Nov 11 '25 22:11 nobodywasishere

I haven't seen any if you can link to one

https://github.com/SchemaStore/schemastore/pull/5122

Sija avatar Nov 11 '25 22:11 Sija

Ahhh okay this is possible. vtesttree.yaml is an example to follow.

image image

nobodywasishere avatar Nov 12 '25 04:11 nobodywasishere

https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md#how-to-add-a-json-schema-with-multiple-versions

nobodywasishere avatar Nov 12 '25 04:11 nobodywasishere

Nice! In such a case, I'd generate the schema for v1.6.4 first and host it in the schemastore repo to see how it works, so if any changes are needed, we'll have it ironed out before v1.7.0, wdyt?

Sija avatar Nov 12 '25 04:11 Sija

https://github.com/SchemaStore/schemastore/pull/5131

nobodywasishere avatar Nov 12 '25 04:11 nobodywasishere

I'd prefer hosting versioned schemas (all except master) in the schemastore repo to avoid keeping lingering branches.

Sija avatar Nov 12 '25 13:11 Sija

Relevant configuration settings.json, allowing for testing the feature until it's officially supported.

VS Code

  "yaml.schemas": {
    "https://raw.githubusercontent.com/crystal-ameba/ameba/master/.ameba.yml.schema.json": ".ameba.yml"
  }

Zed

  "lsp": {
    "yaml-language-server": {
      "settings": {
        "yaml": {
          "schemas": {
            "https://raw.githubusercontent.com/crystal-ameba/ameba/master/.ameba.yml.schema.json": [
              "/.ameba.yml"
            ]
          }
        }
      }
    }
  }

Sija avatar Nov 15 '25 18:11 Sija