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 YAML for human-edited metadata (YAMLKAN) #3367

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 14, 2021

Motivation

Currently all metadata is in JSON. This is fine/great for machine-written metadata, but for humans it can be a bit tricky, as we have to worry about proper usage of quotes, commas, braces, brackets, etc.

YAML is a JSON superset format used for GitHub Actions and many other administrative/automation contexts. It is somewhat more human-friendly than JSON, as quotes, commas, braces, and brackets are assumed based on the structure and indenting of the data. For example, this JSON:

{
   "spec_version": "v1.4",
   "identifier": "Cosmogator",
   "$kref": "#/ckan/github/HebaruSan/Astrogator",
   "license": "GPL-3.0",
   "tags": [
      "plugin",
      "information",
      "control"
   ],
   "install": [
      {
         "find": "Astrogator",
         "install_to": "GameData"
      }
   ]
}

is equivalent to this YAML:

spec_version: v1.4
identifier:   Cosmogator
$kref:        "#/ckan/github/HebaruSan/Astrogator"
license:      GPL-3.0
tags:
    - plugin
    - information
    - control
install:
    - find: Astrogator
      install_to: GameData

https://www.convertjson.com/yaml-to-json.htm

To cure our "notoriously resistance on implementing new features on CKAN" 🤣, we should be adventurous and always try new things. 🏴‍☠️ ⛷️ 🤹

Changes

Now the human-maintained parts of the metadata (the things that Netkan treats as inputs) are allowed to be in JSON or YAML (since all valid JSON is also valid YAML):

  • Netkans
  • Meta-netkans
  • Internal ckans

Netkan's output is still exclusively JSON, and none of the other JSON files that we save are affected (config, registry, etc.). The client still only deals in JSON. Tests are included to make sure our YAML parsing and JSON conversion are correct.

  • I forgot to update the spec to indicate that YAML is allowed for .netkans

Future to-do

To deploy this fully, a few more changes will be needed in other repos:

@HebaruSan HebaruSan added Pull request In progress We're still working on this Spec Issues affecting the spec Netkan Issues affecting the netkan data Tests Issues affecting the internal tests labels May 14, 2021
@HebaruSan HebaruSan requested a review from DasSkelett May 14, 2021 23:04
@HebaruSan HebaruSan added Enhancement New features or functionality and removed In progress We're still working on this labels May 14, 2021
@techman83
Copy link
Member

Have not reviewed the code, but I'm a very strong supporter of YAML where humans need to be involved. JSON is not human friendly.

@HebaruSan
Copy link
Member Author

I just realized I forgot to add the "discussion needed" label, as it is very much not yet set in stone whether we would want to do this. Thanks for your two cents, @techman83.

@techman83
Copy link
Member

No worries. Not that this is on the table, but CKAN-meta should remain json only, it is preferable to let NetKAN do all the heavy lifting. But the amount of issues we've had with reasonable JSON mistakes over the years!

@HebaruSan
Copy link
Member Author

CKAN-meta should remain json only, it is preferable to let NetKAN do all the heavy lifting

Yeah, agreed. I will not be submitting my other branch that was converting everything to YAML. Not only are there way more changes needed, but some of the existing JsonProperty tricks are hard to port over, and the OnDeserialized attribute is completely different (if available at all, not sure).

@DasSkelett
Copy link
Member

Executing netkan.exe for the JSON Astrogator.netkan:

$ mono netkan.exe Astrogator.netkan 
1827 [1] FATAL CKAN.NetKAN.Program (null) - Schema validation failed: #/spec_version: NotOneOf

It converts "spec_version": 1 to "spec_version": "1" in metadata.Json(), which is not valid according to our schema. Can we somehow tell it to convert "spec_version": 1 to an int, as it is in the original JSON?

@HebaruSan
Copy link
Member Author

I know how to generate int values for strings that are parseable as ints, done in latest commit. Can you think of a way this could go wrong?

@DasSkelett
Copy link
Member

Maybe author names that are numeric symbols / digits only? Weirdly enough, we have at least two:
image

Also folder/file names in install stanzas, but I hope nobody's actually giving them digit-only names.
Theoretically the version attribute could also be a simple number and they're allowed in netkans, but they shouldn't be in there, not sure if we need to worry about them.

@HebaruSan
Copy link
Member Author

Luckily the parser keeps track of the original syntax for us. So if a field gives us trouble, after the latest commit we can change this:

author: 123456

to this:

author: "123456"

@DasSkelett
Copy link
Member

A small side-effect I just encountered: People need to update their file type associations in their editors ^^
And GitHub's repo language stats might get a little out of sync again, but that's not a biggie:
https://github.com/KSP-CKAN/NetKAN/blob/master/.gitattributes
I mean we could jsut set it to YAML, which would be technically true but slightly misleading.

That aside, works pretty good so far in my testing. I'm also going to try to write some YAML netkans from scratch to find possible trouble spots that need mentioning in a guide.

@HebaruSan
Copy link
Member Author

People need to update their file type associations in their editors ^^

Indeed, my vim really does not like my Comsogator.netkan, almost the whole thing is highlighted red. atom also very confused.

And GitHub's repo language stats might get a little out of sync

Good point, will add to post-merge steps...

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

I think this is good. I tested YAML and JSON netkans, and they all seem to work fine and produce the same output as before.
Let's give this a go, for hopefully fewer syntax issues in the future 🤞

@HebaruSan HebaruSan merged commit 8f59934 into KSP-CKAN:master Jun 10, 2021
@HebaruSan HebaruSan deleted the feature/yaml-netkan branch June 10, 2021 16:55
@HebaruSan
Copy link
Member Author

One of the tests queries SpaceDock, so it's failing for now because SpaceDock is having a relapse of KSP-SpaceDock/SpaceDock#226. Will have to re-run the Actions once it's working again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion needed Enhancement New features or functionality Netkan Issues affecting the netkan data Spec Issues affecting the spec Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants