Validators #5

sr480 · 2023-10-02T18:42:32Z

Валидация Meta и Features
Добавил форматы ошибок валидаторов,
Добавил в cli команду validate

sr480 · 2023-10-02T18:43:52Z

package.json

@@ -9,6 +9,7 @@
        "dist"
    ],
    "scripts": {
+        "start": "NODE_TLS_REJECT_UNAUTHORIZED=0 ts-node ./src/cli.ts",


добавил команду для запуска без аргументов, запускаю проверку вот так: npm start -- validate

sr480 · 2023-10-02T18:44:58Z

src/commands/validate-only.ts

+
+    const meta = await loadMeta(yml.metaPath, projectPath);
+
+    const metaValidation = validateMeta(meta);


Отдельный набор ошибок по валидации Meta, тут же строю мапу для атрибутов-значений

Может стоит останавливать валидацию, если есть ошибки в Meta?

sr480 · 2023-10-02T18:46:34Z

src/commands/validate-only.ts

+    );
+
+    console.log(metaValidation);
+    console.log(featureValidation);


Хочу получить ревью, до того как буду делать красивый вывод ошибок

sr480 · 2023-10-02T18:53:31Z

src/lib/domain/index.ts

+    const component = attributes?.component.join("-");
+    const subComponent = attributes?.["sub-component"].join("-");
+    code = `${component}_${subComponent}`;
+  }


Пришлось переписать, так как в моих yml нет таких атрибутов

я планировал этот код выпилить сегодня и брать значение только из поля code (сделать его обязательным)
через пару часов пришлю PR

sr480 · 2023-10-02T18:54:26Z

src/lib/domain/validators.ts

+  attributeValuesMap: AttributeValuesMap
+): ValidationError[] => {
+  const errors = new Array<ValidationError>();
+  const featureCodesUnique = new Map<string, Feature>();


Строю мап фичей по кодам, чтобы проверять дубликаты и указывать на фичу первым встретившимся кодом

sr480 · 2023-10-02T19:00:23Z

src/lib/validators.ts

+  | FeatureMissingAttributeError
+  | FeatureMissingAttributeValueError
+  | CodeError;
+


Не добавлял filePath в ошибки, так как он содержится в Feature

sr480 · 2023-10-05T08:36:28Z

src/lib/validators/index.ts

+
+    if (feature.attributes) {
+      for (const attribute in feature.attributes) {
+        let valuesSet = this.metaAttributeValues.get(attribute);


Нужно ли проверять валидность кода атрибута? Все-равно выпадет ошибка, так как он не будет найден в мета, а там идет проверка формата кода атрибута.

в месте использования валидность кода атрибута проверять не нужно — только в месте объявления атрибута

код значения нужно проверять, т.к. значения можно не объявлять в мете

sr480 · 2023-10-05T08:40:13Z

src/lib/yaml/index.ts

-): Promise<YamlFile> => {
+  basePath: string = CWD, // TODO: перенести в resolvePath
+  validationContext?: Validator
+): Promise<YamlFile | undefined> => {


я решил возвращать undefined, если отловил ошибку. из-за этого снаружи приходится фильтровать файлы перед последующей обработкой

sr480 · 2023-10-05T08:40:48Z

src/commands/sync.ts


-    const projectData = processYamlFiles(yamls, meta);
+    const projectData = processYamlFiles(successYamls, meta);


Пришлось добавить код выбора только успешно декодированных yaml

sr480 · 2023-10-05T08:53:40Z

src/lib/config/index.ts

+        'config'
+      );
+    } else {
+      throw error;


если не переда кнтекст валидации, то валимся с исключением, так как некому собирать ошибки

предлагаю сделать контекст валидации обязательным параметром, чтобы логика работы sync и validate отличалась только наличием в конце операции выгрузки данных

sr480 · 2023-10-05T08:55:15Z

src/lib/yaml/index.ts

+    };
+  } catch (error) {
+    if (validationContext) {
+      validationContext.registerLoaderError(error, relativePath, 'feature');


тут аналогично, либо собираем ошибки в конексте валидации либо валимся с исключением

dima117 · 2023-10-03T07:37:21Z

src/lib/domain/validators.ts

+      };
+      errors.push(featureCodeError);
+    }
+  }


предлагаю вынести валидацию формата кода во всех сущностях на уровень проверки схемы

dima117 · 2023-10-03T07:43:44Z

src/lib/domain/validators.ts

+        feature,
+        firstFeature: existingFeature,
+      };
+      errors.push(featureCodeDuplicateError);


код может повторяться несколько раз
лучше бы выдавать ошибку вида:
повторяется код <код> в файлах: [ ... ]

dima117 · 2023-10-03T07:46:34Z

src/lib/domain/validators.ts

+        errors.push(missingAttribute);
+        continue;
+      }
+      for (const attributeValue of feature.attributes[attribute]) {


проверять наличие всех значений не нужно, т.к. частая ситуация, когда есть большое количество значений и они не описаны в конфиге — считаем, что это нормально и при выгрузке автоматом добавляем в базу недостающие занчения

с атрибутами так не прокатит, т.к. мы ссылаемся на атрибуты из других мест (например, из деревьев), поэтому проверяем наличие атрибутов

dima117 · 2023-10-03T07:47:35Z

src/lib/domain/index.ts

@@ -91,3 +94,5 @@ export const processYamlFiles = (

  return { features, attributes, trees };
 };
+
+export * from './validators';


предлагаю все валидаторы вынести в отдельную папку validation на одном уровне с папками config и domain

dima117 · 2023-10-05T14:33:15Z

src/commands/sync.ts

@@ -21,8 +21,10 @@ export const cmdSync: CommandModule<{}, CommonOptions> = {
    const yamls = await Promise.all(
      files.map((path) => loadYaml(path, projectPath))
    );
+    const successYamls = new Array<YamlFile>();
+    yamls.forEach(yaml => yaml && successYamls.push(yaml));


предлагаю написать

const successYamls = yamls.filter(Boolean)

не получится, тогда нужно еще map делать, так как тип у массива останется (YamlFile | undefined)[]

:(

dima117 · 2023-10-05T14:36:43Z

src/lib/config/index.ts

+        'config'
+      );
+    } else {
+      throw error;


предлагаю сделать контекст валидации обязательным параметром, чтобы логика работы sync и validate отличалась только наличием в конце операции выгрузки данных

dima117 · 2023-10-05T14:54:23Z

src/lib/validators/index.ts

+
+    if (feature.attributes) {
+      for (const attribute in feature.attributes) {
+        let valuesSet = this.metaAttributeValues.get(attribute);


в месте использования валидность кода атрибута проверять не нужно — только в месте объявления атрибута

код значения нужно проверять, т.к. значения можно не объявлять в мете

dima117 · 2023-10-05T14:56:34Z

src/lib/validators/index.ts

+
+export class Validator {
+  private readonly featuresMap = new Map<string, Feature>();
+  private readonly metaAttributeValues = new Map<string, Set<string>>();


немного смущает, что featuresMap и metaAttributeValues находятся в полях класса, к ним есть обращения из методов, но в момент обращения эти значения могут быть не заполнены данными

предлагаю объявить их как локальные переменные методов и явно передавать во вложенные методы как параметры

т.к. validateMeta и validateFeatures используются ровно в одном месте и вызываются вместе, то предлагаю объединить их в один метод и сделать featuresMap и metaAttributeValues его локальными переменными

также этот метод можно вынести в родителя из processYamlFiles

также вместо параметров meta и fetures в него можно передавать один параметр типа ProjectData, который содержит всю ту же самую информацию (т.е. передать в него возвращаемое значение от метода processYamlFiles)

dima117 · 2023-10-11T04:21:25Z

src/lib/config/index.ts

    return content || {};
+  } catch (error) {
+    validationContext.registerLoaderError(error, filePath, 'config');


если не загрузилась мета, нужно падать с ошибкой, т.к. без этого точно не пройдет валидация остальных файлов

dima117 · 2023-10-11T04:56:31Z

src/lib/validators/index.ts

+    | 'description'
+    | 'title'
+    | 'group.title'
+    | 'group.description'


вроде бы у группы нельзя задать description

dima117 · 2023-10-11T05:15:52Z

src/lib/validators/index.ts

+    const links = LINK_LIKE.exec(value);
+    if (links) {
+      for (let link of links) {
+        link = link.substring(1);


если regexp изменится, то никак нельзя догадаться, что нужно пойти в это место и поправить логику
предлагаю сделать группу в regex и вместо substring выбирать группу + оставить комментарий об этом в месте, где описан regex

dima117 · 2023-10-11T05:28:47Z

src/lib/validators/index.ts

+import { Assertion, AssertionGroup, Feature } from '../domain';
+import { printError } from './renderer';
+
+export type AttributeDuplicateError = {


предлагаю вынести валидатор в отдельный файл + вынести описание интерфейсов в файл model.ts (по аналогии с другими частям проекта), а в index.ts оставить только экспорт элементов, которые используются снаружи

dima117 · 2023-10-11T05:31:31Z

src/lib/jest/index.ts

@@ -50,14 +56,20 @@ export const applyJestReport = (
        const parts = getKey(keyParts, assertionCtx, attributesCtx);
        const fullName = getFullName(...parts);

-        console.log("process:  \x1b[36m%s\x1b[0m", fullName);
+        console.log(`process:  ${chalk.cyan(fullName)}`);


предлагаю этот console.log убрать совсем — он был нужен, когда не было нормальной валидации, чтобы смотреть какие тесты не сматчились на фичи

dima117 · 2023-10-11T05:36:45Z

src/commands/sync.ts

    const { yml, api, jest, projectPath } = await loadConfig(args.config);

-    const meta = await loadMeta(yml.metaPath, projectPath);
+    const meta = await loadMeta(validationContext, yml.metaPath, projectPath);
    const files = await glob(yml.files, { cwd: projectPath });

    const yamls = await Promise.all(
      files.map((path) => loadYaml(path, projectPath))


кажется, нужно в loadYaml передать validationContext

dima117 · 2023-10-11T17:33:52Z

src/lib/validators/renderer.ts

@@ -1,50 +1,55 @@
 import chalk from 'chalk';
-import { ERROR_SEVERITY, ErrorSeverity, ValidationError, ValidationErrorTypes } from './models';
+import { ERROR_SEVERITY, ErrorSeverity, ValidationError } from './models';

 const renderError = (e: ValidationError): string => {
  switch(e.type){


изменен формат вывода ошибок:

добавил разделение сообщений пустой строкой и яркий маркер начала нового сообщения

сделал начало сообщений на одном уровне (раньше начало сообщения зависело отдлины пути к файлу)

покрасил путь к файлу в серый (чтобы привлекал меньше внимания)

вместо нескольких цветов все значения вывожу одним цветом

dima117 · 2023-10-11T17:34:35Z

src/lib/validators/renderer.ts

  }
 }

+const path = (p: string) => chalk.gray(p);
+const strong = (p: string) => chalk.bold(p);
+const val = (p: string) => chalk.cyan(p);


сделал семантические хелперы, чтобы при изменении формата он единообразно менялся во всех сообщениях

классно!

dima117 · 2023-10-11T17:35:32Z

src/lib/validators/validator.ts

@@ -129,7 +128,8 @@ export class Validator {
  ): ValidationError[] {
    const result = new Array<ValidationError>();
    for (const attributeValue of attribute.values) {
-      const valueValidation = this.validateCode(attribute.code, filePath);
+      const valueValidation = this.validateCode(attributeValue.code, filePath);


была опечатка — вместо кода значения проверялся код атрибута

dima117 · 2023-10-11T17:35:56Z

src/lib/validators/validator.ts

      for (const tree of trees) {
-        const treeUnique = new Set<string>();


была опечатка — на каждом шаге цикла создавался новый set

dima117 · 2023-10-11T17:36:45Z

src/lib/validators/validator.ts

-        };
-        errors.push(groupDuplicateError);
-      }
-      groupsUnique.add(assertionGroup.title);


выпилил эту проверку, т.к. список групп описывается как dictionary и если задублируются значения групп, то упадет проверка формата

ага, я видел, что есть в yaml такое ограничение, но подумал оставить - вдруг формат yaml решим поменять, а для БД это критично

sr480 · 2023-10-11T17:43:31Z

src/lib/validators/models.ts

@@ -78,12 +78,7 @@ export type FeatureMissingLinkError = {
    | 'assert.description';
  link: string;
 };
-export type AssertionGroupDuplicateError = {


Разве не стоит считать дубли групп ассертов ошибками?

по идее, это ошибка, но уже есть два места, которые её обрабатывают:

упадет разбор формата

на сервере при экспорте одинаковые группы склеятся в одну

кажется, если мы добавим еще одну проверку, то в реальной жизни она никогда не сработает (и если она сломается, мы об этом не узнаем)

т.е. суть предложения — не оставлять в проекте неиспользуемый код

sr480 · 2023-10-11T17:45:06Z

src/lib/validators/renderer.ts

  }
 }

+const path = (p: string) => chalk.gray(p);
+const strong = (p: string) => chalk.bold(p);
+const val = (p: string) => chalk.cyan(p);


классно!

sr480 · 2023-10-11T17:47:24Z

src/lib/validators/validator.ts

-        };
-        errors.push(groupDuplicateError);
-      }
-      groupsUnique.add(assertionGroup.title);


ага, я видел, что есть в yaml такое ограничение, но подумал оставить - вдруг формат yaml решим поменять, а для БД это критично

dima117 · 2023-10-11T17:56:02Z

src/lib/config/index.ts


+  try {
+    validationContext.setMetaFilePath(filePath);


выпилить setMetaFilePath?

dima117 · 2023-10-11T17:57:26Z

src/lib/config/index.ts

    return content || {};
+  } catch (error) {
+    printError(getLoaderError(error, filePath, 'config'));
+    throw 'Ошибка загрузки файла конфигурации';


давай, пожалуйста, в throw всегда будем передавать объект Error?

throw new Error('Ошибка загрузки файла конфигурации');

dima117 · 2023-10-11T18:03:12Z

src/lib/validators/validator.ts

+  private metaFilePath = '';
+
+  get hasCriticalErrors(): boolean {
+    let hasError = false;


предлагаю написать так:

let hasError = [ ...loaderErrors, ...metaErrors, ...featureErrors, ...jestUnusedTests].some(e => ERROR_SEVERITY[e.type] === 'error');

sr480 force-pushed the validators branch 2 times, most recently from 0ad9187 to e1fdbbd Compare October 2, 2023 18:59

sr480 commented Oct 2, 2023

View reviewed changes

validators

a37de64

sr480 force-pushed the validators branch from e1fdbbd to afcaab7 Compare October 5, 2023 08:27

sr480 commented Oct 5, 2023

View reviewed changes

sr480 force-pushed the validators branch from afcaab7 to d62fde8 Compare October 5, 2023 08:51

sr480 commented Oct 5, 2023

View reviewed changes

dima117 reviewed Oct 5, 2023

View reviewed changes

sr480 force-pushed the validators branch 2 times, most recently from c49d2a8 to 18fcd04 Compare October 9, 2023 13:53

dima117 reviewed Oct 11, 2023

View reviewed changes

validators

be663e6

sr480 force-pushed the validators branch from 18fcd04 to be663e6 Compare October 11, 2023 10:51

dima117 reviewed Oct 11, 2023

View reviewed changes

sr480 commented Oct 11, 2023

View reviewed changes

dima117 reviewed Oct 11, 2023

View reviewed changes

sr480 force-pushed the validators branch 2 times, most recently from 820a41f to 72eb499 Compare October 11, 2023 19:27

добавлена статистика ошибок

472ed1f

sr480 force-pushed the validators branch from 72eb499 to 472ed1f Compare October 11, 2023 19:38

dima117 merged commit 3defa72 into master Oct 11, 2023

dima117 deleted the validators branch October 11, 2023 20:08


		const meta = await loadMeta(yml.metaPath, projectPath);

		const metaValidation = validateMeta(meta);


		const projectData = processYamlFiles(yamls, meta);
		const projectData = processYamlFiles(successYamls, meta);

		for (const tree of trees) {
		const treeUnique = new Set<string>();

Validators #5

Validators #5

Conversation

sr480 commented Oct 2, 2023

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

dima117 Oct 3, 2023 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

dima117 Oct 3, 2023 •

edited

Loading