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

use json schema and validation for configuration #145

Merged
merged 14 commits into from
Jun 22, 2021

Conversation

DanielHabenicht
Copy link
Contributor

@DanielHabenicht DanielHabenicht commented Apr 25, 2021

Here is my draft for using a schema instead of Markdown to document the ctf.json.

It also validates part of the JSON with the Schema (only those parts supported by the JSONschema).

I also fixed the files checked in for .net (e.g. launchSettings.json which should be shared for an easier setup).

@DanielHabenicht DanielHabenicht changed the title use json schema and validation for configuration Draft: use json schema and validation for configuration Apr 25, 2021
@Savallator
Copy link
Contributor

Can you please not mix up several different issues in one pull request?
Also, the changes for the gitignore are not helpful at all.

@DanielHabenicht
Copy link
Contributor Author

DanielHabenicht commented Apr 25, 2021

Can you please not mix up several different issues in one pull request?

yes, sorry will update it.

Also, the changes for the gitignore are not helpful at all.

They are:

  1. launchSettings.json should be shared so that ports etc. are standardized for all developers
  2. Instead of adding every file by hand using the standard predefined .gitignore from Microsoft will ensure that nothing gets included/excluded that shouldn't be instead of googling every new file type. (e.g. the previous setting of excluding the whole Properties folder and cproj.user files not being excluded currently).

@Savallator
Copy link
Contributor

We did use a standard predefined .gitignore. Also e.g. removing the ctf.*.json there is not helpful. Maybe you should get some experience with actually using the software first so you know some of the basic usecases?

@Savallator
Copy link
Contributor

Also, could you please not remove the whitespaces we put into the readme for readability?

@DanielHabenicht
Copy link
Contributor Author

We did use a standard predefined .gitignore. Also e.g. removing the ctf.*.json there is not helpful. Maybe you should get some experience with actually using the software first so you know some of the basic usecases?

I agree with that. But also you have ctf.bambi3.json and more checked into the repository, so it is kind of contradictory.

Also, could you please not remove the whitespaces we put into the readme for readability?

Yes, that was my autoformatter.

@Savallator
Copy link
Contributor

I agree with that. But also you have ctf.bambi3.json and more checked into the repository, so it is kind of contradictory.

Those were checked in deliberately by hand. But you don't want to get every test file in there.

@DanielHabenicht
Copy link
Contributor Author

Those were checked in deliberately by hand. But you don't want to get every test file in there.

Then maybe move these files into an /archive folder? and exclude it from git.

@Trolldemorted
Copy link
Member

Then maybe move these files into an /archive folder? and exclude it from git.

Then we would lose track of which services were played when, which is bad for servicerecyclingcoin, but we are currently considering moving them into a dedicated repository together with dumps of their databases.

@DanielHabenicht DanielHabenicht changed the title Draft: use json schema and validation for configuration use json schema and validation for configuration Apr 25, 2021
@DanielHabenicht
Copy link
Contributor Author

updated as you requested.

@DanielHabenicht
Copy link
Contributor Author

DanielHabenicht commented Apr 27, 2021

I don't know who was sad about using Newtonsoft again, but I found a package which extends the built in JSON Serializer. (https://gregsdennis.github.io/json-everything/usage/schema-generation.html)

If someone wants to comment on whether you actually want to document it as JSON Schema I can remodel it to use the other package.

"description": "The length of one round in seconds.",
"type": "integer",
"minimum": 1.0,
"maximum": 9.223372036854776E+18
Copy link
Member

@domenukk domenukk Apr 27, 2021

Choose a reason for hiding this comment

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

Is this schema autogenerated? Else we could do more useful maxima than "21x age of the universe" seconds :D
Obviously not a big deal for now, just wondering :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's auto-generated. I made the core package executable for that purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you can put in any number you want in the .cs file describing the entity.

@@ -1,259 +1,165 @@
namespace EnoCore.Configuration
Copy link
Member

Choose a reason for hiding this comment

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

Why does github display this as a diff? Did the linefeeds change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like Visual Studio loves CRLF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but for me it shows up as LF..

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it might be possible to enforce one kind of line endings project-wide, at least i read about that somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise it will depend on the settings of the one editing it, which is bad imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but with .gitattributes I didn't see any changes

Copy link
Member

Choose a reason for hiding this comment

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

There is e.g. .editorconfig, but with VS it was a clusterfuck a few years back - if you specify \n linefeeds then new classes will have \r\n boilerplate code, and any linefeeds you enter will be \n only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I worked with the other projects I would say that the file probably was CRLF and now is LF only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can run a formater like https://restyled.io/ on all project if you like. I can set it up but you need to allow the application.

Copy link
Member

@ldruschk ldruschk left a comment

Choose a reason for hiding this comment

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

not sure what the current state on the discussion of LF vs CR+LF is, but apart from that looks good

@DanielHabenicht
Copy link
Contributor Author

DanielHabenicht commented Jun 21, 2021

not sure what the current state on the discussion of LF vs CR+LF is, but apart from that looks good

As far as I know the file was checked in as CRLF (current master) an is now LF (my PR).
(Other files might still be checked in as CRLF so I think it might be a good idea to incorporate https://restyled.io/ I can set it up if you'd like, but you will have to install it to the Org.

@ldruschk ldruschk merged commit 4c8f920 into enowars:master Jun 22, 2021
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.

5 participants