[FEATURE] Add Configuration Schema #274

matz3 · 2020-03-12T12:17:41Z

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

Reviewed the Contributing Guidelines
- Especially the How to Contribute section
No merge commits
Correct commit message style

coveralls · 2020-03-12T15:53:38Z

Coverage increased (+1.5%) to 89.812% when pulling 2341c79 on schema into 6b717fd on master.

RandomByte

I like what I see

lib/projectPreprocessor.js

lib/schema/validate.js

matz3 · 2020-03-20T08:38:30Z

Should be rebased on top of #277

svbender · 2020-03-20T10:19:26Z

lib/validation/ValidationError.js

+const escapeStringRegExp = require("escape-string-regexp");
+const matchAll = require("string.prototype.matchall");
+
+class ValidationError extends Error {


I am really missing some jsdoc here for the class and methods.

svbender · 2020-03-20T10:24:44Z

lib/validation/schema/specVersion/2.0/kind/project.json

+	"properties": {
+		"specVersion": { "enum": ["2.0"] },
+		"kind": {
+			"enum": ["project", null]


What is the enum entry "null" for?

The default when it's not defined. I've added comments.

svbender · 2020-03-20T10:31:37Z

lib/validation/validator.js

+	return JSON.parse(schemaFile);
+}
+
+class Validator {


Same here, no documentation. Most of the methods logic is obvious but at least the class could use some doc.

Done for the public API

svbender · 2020-03-20T10:37:25Z

test/lib/projectPreprocessor.js

+	});
+
+	t.is(validationError.errors.length, 1, "ValidationError should have one error object");
+	t.is(validationError.errors.length, 1, "ValidationError should have one error object");


Duplicate assertion

RandomByte · 2020-03-19T19:24:37Z

lib/projectPreprocessor.js

@@ -3,8 +3,9 @@ const fs = require("graceful-fs");
 const path = require("path");
 const {promisify} = require("util");
 const readFile = promisify(fs.readFile);
-const parseYaml = require("js-yaml").safeLoadAll;
+const {loadAll: parseYaml, DEFAULT_SAFE_SCHEMA} = require("js-yaml");


This change is needed because of the issues with safeLoadAll? If yes, maybe add a comment for that.

Yes, good point. Done.

lib/projectPreprocessor.js

RandomByte

I did not look into all the new tests, it's just too much. But that's still a good thing 👍

lib/projectPreprocessor.js

lib/validation/ValidationError.js

lib/validation/schema/specVersion/2.0/kind/extension.json

test/lib/projectPreprocessor.js

test/lib/validation/schema/specVersion/2.0/kind/project/application.js

RandomByte

projectPreprocessor » readConfigFile: Exception for invalid config failed on Windows: https://dev.azure.com/sap/opensource/_build/results?buildId=47033&view=ms.vss-test-web.build-test-results-tab&runId=1017232&resultId=100193&paneView=debug

RandomByte · 2020-03-23T23:31:28Z

lib/validation/validator.js

+	 * @param {number} [options.yaml.documentIndex=0] Document index in case the YAML file contains multiple documents
+	 * @throws {module:@ui5/project.validation.ValidationError}
+	 *   Throws a {@link module:@ui5/project.validation.ValidationError ValidationError} when the validation fails.
+	 * @returns {undefined} Returns when the validation succeeds


Returns a Promise, right?

matz3 · 2020-03-24T08:24:09Z

Tests should be fixed now.

RandomByte · 2020-03-24T14:54:25Z

test/lib/validation/ValidationError.js

+	const yaml = {
+		path: "/my-project/ui5.yaml",
+		source:
+`---


What about one (or multiple) newlines at the beginning of the file?

Good point. Done 👍

RandomByte · 2020-03-24T17:20:08Z

lib/projectPreprocessor.js

-					source: configFile,
-					documentIndex
+			configs.map(async (config, documentIndex) => {
+				try {


We should document that the try/catch block here is required to get the first error of the first document. And not the first error to occur in any document.

…ling

RandomByte

I like it, let's enforce the schema!!!11

🚓 👮 👮‍♀ 🚔

KlattG

Whenever errors are triggered, the word "should" is not strong enough. Please replace "should" by "must" in all error messages.

KlattG · 2020-03-26T07:49:09Z

test/lib/validation/ValidationError.js

+		schemaPath: "#/type",
+	};
+
+	const expectedErrorMessage = `Configuration ${chalk.underline(chalk.red("foo"))} should be of type 'object'`;


"should" --> "must"

KlattG · 2020-03-26T07:50:09Z

test/lib/validation/ValidationError.js

+		schemaPath: "#/required",
+	};
+
+	const expectedErrorMessage = "Configuration should have required property 'specVersion'";


"Configuration must have the required property 'specVersion'"

KlattG · 2020-03-26T07:51:06Z

test/lib/validation/ValidationError.js

+		message: "should have required property 'name'"
+	};
+
+	const expectedErrorMessage = `Configuration ${chalk.underline(chalk.red("metadata"))} should have required property 'name'`;


"Configuration ${chalk.underline(chalk.red("metadata"))} must have the required property 'name'"

KlattG · 2020-03-26T07:51:51Z

test/lib/validation/ValidationError.js

+	};
+
+	const expectedErrorMessage =
+`Configuration ${chalk.underline(chalk.red("type"))} should be equal to one of the allowed values


"Configuration ${chalk.underline(chalk.red("type"))} must be equal to one of the allowed values"

KlattG · 2020-03-26T07:53:56Z

test/lib/validation/ValidationError.js

+	};
+
+	const expectedErrorMessage =
+`Configuration ${chalk.underline(chalk.red("resources/configuration"))} property propertiesFileEncoding is not expected to be here`;


"Configuration ${chalk.underline(chalk.red("resources/configuration"))} property propertiesFileEncoding must not be provided here"

matz3 · 2020-03-26T15:38:29Z

Thanks @KlattG 👍
Will be addressed with #292

RandomByte reviewed Mar 13, 2020

View reviewed changes

lib/projectPreprocessor.js Outdated Show resolved Hide resolved

lib/schema/validate.js Outdated Show resolved Hide resolved

matz3 force-pushed the schema branch from b82c110 to 2a66684 Compare March 17, 2020 11:48

matz3 requested a review from svbender March 19, 2020 10:58

matz3 marked this pull request as ready for review March 20, 2020 08:38

svbender reviewed Mar 20, 2020

View reviewed changes

RandomByte reviewed Mar 20, 2020

View reviewed changes

matz3 force-pushed the schema branch from 66e5a43 to 56c2627 Compare March 20, 2020 13:29

RandomByte requested changes Mar 22, 2020

View reviewed changes

RandomByte requested changes Mar 23, 2020

View reviewed changes

matz3 force-pushed the schema branch from 2ab3f4d to cb21e15 Compare March 24, 2020 08:33

RandomByte requested changes Mar 24, 2020

View reviewed changes

RandomByte mentioned this pull request Mar 24, 2020

[ROADMAP] Specification Version 2.0 // UI5 CLI 2.0 SAP/ui5-tooling#204

Closed

15 tasks

matz3 added 14 commits March 24, 2020 19:51

[FEATURE] Add Configuration Schema

036b53e

Add more schemas

fb6f502

Use tabs

e68ace7

Fix schema, add more tests + coverage

234ad0a

Restructure schema files

c95927a

Integrate into ProjectPreprocessor / Add ValidationError

7b21994

Add framework schema

bbef4c6

Refactor schema, validate, AjvCoverage

7c480ac

Improve error messages, fix tests, move AjvCoverage

f546bd9

Add validate test

a97dcda

Add filterErrors test, refactor

cf00391

validate.js -> validator.js, add yaml formatting functions

c8d0ba2

Extract ValidationError from validator

61de70f

ValidationError.analyzeYamlError

f092963

matz3 added 21 commits March 24, 2020 19:51

Tests

00c934d

Fix Node 10

21535d9

Schema refactoring / fixes

673d434

Add bundles schema definition

5e2c638

type module

1724fd0

Improve horizontal separator line

60b2d99

Fix tests on Windows

3473e94

Throw error for root project when ui5.yaml can't be read

584c500

Also validate shim configuration

558dceb

Remove test

9e990fe

Code review changes part 1

08776eb

Fix ui5framework.integration.js

1e72cc9

Code review changes part 2

186a3a2

Code review changes part 3

7df1980

analyzeYamlError: support multiple documents

bed154b

Add schema for builder/componentPreload

581e939

Fix yaml documentIndex logic

db5d4ec

Fix another test on windows

8bf7f6d

Support Windows Line-Endings

de84807

projectPreprocessor readConfigFile: Add more tests and fix error hand…

6f32895

…ling

Update coverage thresholds / Add comment

3a4329e

RandomByte previously approved these changes Mar 24, 2020

View reviewed changes

Add latest changes after resolving conflicts...

2341c79

matz3 dismissed RandomByte’s stale review via 2341c79 March 24, 2020 18:58

matz3 force-pushed the schema branch from c8aab6d to 2341c79 Compare March 24, 2020 18:58

RandomByte approved these changes Mar 24, 2020

View reviewed changes

matz3 merged commit eb961c3 into master Mar 24, 2020

matz3 deleted the schema branch March 24, 2020 20:01

KlattG reviewed Mar 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Add Configuration Schema #274

[FEATURE] Add Configuration Schema #274

matz3 commented Mar 12, 2020

coveralls commented Mar 12, 2020 •

edited

Loading

RandomByte left a comment

matz3 commented Mar 20, 2020

svbender Mar 20, 2020

matz3 Mar 23, 2020

svbender Mar 20, 2020

matz3 Mar 20, 2020

svbender Mar 20, 2020

matz3 Mar 23, 2020

svbender Mar 20, 2020

matz3 Mar 20, 2020

RandomByte Mar 19, 2020

matz3 Mar 20, 2020

RandomByte left a comment

RandomByte left a comment

RandomByte Mar 23, 2020

matz3 Mar 24, 2020

matz3 commented Mar 24, 2020

RandomByte Mar 24, 2020

matz3 Mar 24, 2020

RandomByte Mar 24, 2020

matz3 Mar 24, 2020

RandomByte left a comment

KlattG left a comment

KlattG Mar 26, 2020

KlattG Mar 26, 2020

KlattG Mar 26, 2020

KlattG Mar 26, 2020

KlattG Mar 26, 2020

matz3 commented Mar 26, 2020

[FEATURE] Add Configuration Schema #274

[FEATURE] Add Configuration Schema #274

Conversation

matz3 commented Mar 12, 2020

Pull Request Checklist

coveralls commented Mar 12, 2020 • edited Loading

RandomByte left a comment

Choose a reason for hiding this comment

matz3 commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte left a comment

Choose a reason for hiding this comment

RandomByte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matz3 commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte left a comment

Choose a reason for hiding this comment

KlattG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matz3 commented Mar 26, 2020

coveralls commented Mar 12, 2020 •

edited

Loading