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

[WIP] JsonSchema Validation #26

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

[WIP] JsonSchema Validation #26

wants to merge 12 commits into from

Conversation

vjspranav
Copy link
Collaborator

Copy link
Collaborator

@raj-vlabs raj-vlabs left a comment

Choose a reason for hiding this comment

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

I was thinking if it's worth to provide functionality for testing multiple files in one go. For example:
validate-schema file1.json file2.json
or
validate-schema file1.json file2.json schema.json

I think it will be too much work for not so much of utility. For this we probably have to specify a flag to indicate if the argument is a file or a schema, which will be too much work unless we use a library.
Can you quickly spend some time in finding out any node libraries which would read and parse command line arguments easily?

@@ -0,0 +1,43 @@
const Ajv = require('ajv');
const path = require('path');
const schemaMap = require('./schemaMap.json')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does using ES6 style imports work with this version of node? If it does, please use that instead of the commonjs require.

json = process.argv[2];
json = json.replace('/', '\\')
let n = json.lastIndexOf('\\');
let jsonKey = json.substring(n + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of string processing, please find a function in the fsx (file system) module which will take a path and will give you back the file name.

validateSchema(json, schema);
break
default:
console.log("Error")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be good to print the correct usage information here instead of just printing Error.
Usage:

* schemaMap.json -> schema-map.json
* npm run validate-schema -f prestest.json
* npm run validate-schema -f pretest.json posttest.json
* npm run validate-schema -f test.json -s ./schemas/quizv2Schema.js
* Support for conditional version
* Reference schema from external files
validation/validate.js Show resolved Hide resolved
case 3:
json = process.argv[2];
json = json.replace("/", "\\");
let n = json.lastIndexOf("\\");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use path or some function in fsx module to get the filename from a pathname.

validateSchema(json, schema);
break;
default:
console.log("Error");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default case should print the usage options

correctAnswer: {
type: "string",
},
difficulty: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to limit the possible values of difficulty to the three values

type: "object",
},
correctAnswer: {
type: "string",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be only one of a b c or d, you need to restrict the scope of possible values.

let filepath = path.resolve(argv.files[0]);
if (!fs.existsSync(filepath)) {
console.log(filepath);
throw new Error("File does not exist");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put the full filepath in the error message so the user knows which file doesn't exist

properties: {
version: {
type: "integer",
enum: [2],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to specify the full version number (like 2.1) son anyone reading the code gets the idea that the version can be in major.minor format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup sir agreed, changed accordingly.

errorMessage: "Version should be 2.0",
},
},
if: { properties: { version: { const: 2 } }, required: ["version"] },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of writing an if clause for each new version, can we move the version:schema map into a separate object so we do not need to touch the code in order to add a new version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we had discussed, the schema doesn't allow me to properly implement that.

@@ -4,8 +4,14 @@ const AjvErrors = require("ajv-errors");
const path = require("path");
const fs = require("fs");
const schemaMap = require("./schema-map.json");
const qv1Schema = require("./schemas/qv1.json");
const qv2Schema = require("./schemas/qv2.json");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same suggestion as above. if we move the version to schema map in a separate variable, we can just import the relevant file based on the version and when we add another version in future, we will not need to touch this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll convert this into an array inclusion

},
"answers": {
"type": "object",
"minProperties": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd think minimum no of answers should be 2, at least one incorrect. Do you see a use case for only the correct answer?

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