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

fix: improve errors for configs #637

Merged
merged 1 commit into from
Nov 28, 2023
Merged

fix: improve errors for configs #637

merged 1 commit into from
Nov 28, 2023

Conversation

shamsartem
Copy link
Collaborator

No description provided.

@shamsartem shamsartem self-assigned this Nov 24, 2023
@shamsartem shamsartem added the e2e label Nov 24, 2023
@shamsartem shamsartem enabled auto-merge (squash) November 24, 2023 16:43
@shamsartem shamsartem merged commit 5afc8f5 into main Nov 28, 2023
13 checks passed
@shamsartem shamsartem deleted the improve-config-errors branch November 28, 2023 09:06
const paramsMessage = yamlDiffPatch("", {}, params);
return `${color.yellow(instancePath)} ${
const prev = errors[i - 1];
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 prevError would be better naming

return "";
}

return `${instancePath === "" ? "" : `${color.yellow(instancePath)} `}${
Copy link
Contributor

@akim-bow akim-bow Nov 28, 2023

Choose a reason for hiding this comment

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

Probably intancePath && color.yellow...would be simpler? Or you have other reason to use ternary here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Linter forbids using strings in the places where bool is expected to prevent unexpected behavior and to be more explicit about what exactly the check is checking

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because e.g. sometimes empty string is valid and you want to get rid of only undefined case

message ?? ""
}\n${paramsMessage}`;
}${paramsMessage === "" ? "" : `\n${paramsMessage}`}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

})
.filter((s) => {
return (
s !== "" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

s && s !== "must match..."

return (
s !== "" &&
s !== "must match exactly one schema in oneOf\npassingSchemas: null\n"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if error messafe in ajv changes in the next version? Probably we can specify this explicitly in ajv error message if possible and refactor this as a shared const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the error changes then user will start seeing a new useless error, I am removing it here only because it's useless. I don't think moving it to const will help, but probably writing a test could, but I think writing a test for this would be too much, this bit is not so important

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can move it to const just for little bit of documentation purpose

Copy link
Contributor

Choose a reason for hiding this comment

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

But can you control this error on ajv? E.g. intercept it before error reaches the user. For example in js-client i use zod which allows me to intercept errors by filtering their codes and classes, change their message and even silence them.
Zod error API.
Because if i understand the code correctly, you want to prevent certain kind of errors from getting in user console.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe possible using this https://ajv.js.org/packages/ajv-errors.html
not sure

Copy link
Contributor

@akim-bow akim-bow Nov 28, 2023

Choose a reason for hiding this comment

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

Maybe you can give it a try sooner or later with TODO and separate task. Dunno how hard would be to rewrite such places (if it even possible).

@@ -277,6 +287,9 @@ export function getReadonlyConfigInitFunction<
getSchemaDirPath,
} = options;

// every config schema has a version property by convention
// eslint-disable-next-line
const latestConfigVersion = latestSchema.properties.version.const as string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain this conversion a bit. Can't we extend schema type somehow with this? What is causing the need to do the conversion?

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 actually do extend schema, so it's impossible to pass pass a schema to this function that doesn't have version. But for some reason ajv types are not perfect in this sence and they don't produce correct results. I will improve comment to explains this a bit better. If you have nothing interesting to do you can to resolve as well, I wasnt' able to do it in a clean way

shamsartem added a commit that referenced this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants