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: Object maxProperties error message references wrong value #501

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

jayalfredprufrock
Copy link
Contributor

No description provided.

@jayalfredprufrock
Copy link
Contributor Author

I didn't add this to my commit, but you might want to consider using language in the error message to indicate the "inclusive" nature of the comparison, e.g. "Expected object to have no more than X properties".

@sinclairzx81
Copy link
Owner

@jayalfredprufrock Hey thanks :) This is a good catch!

I didn't add this to my commit, but you might want to consider using language in the error message to indicate the "inclusive" nature of the comparison, e.g. "Expected object to have no more than X properties".

Yeah, I may look at some of the error reporting messages in later versions. To be honest, I'm almost tempted to remove the messages entirely and just work off the ValueErrorType enum. I think to support i18n, it may be easier to just have mapping functions that map the error type + schema to multiple languages. Occasionally I get issues posted like #500 where users ask about custom error reporting, so just making the mapping a requirement would solve both custom and i18n requirements (but something to look into for another day) :)

Thanks again for picking up on this, will merge across and publish a revision shortly.

@sinclairzx81 sinclairzx81 merged commit 3dce951 into sinclairzx81:master Jul 18, 2023
9 checks passed
@jayalfredprufrock
Copy link
Contributor Author

FWIW, I support getting rid of the error messages altogether. They have come in handy in the past, but they could be handled just as easily in userland and as you say, that would open the door for i18n as well.

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Jul 18, 2023

@jayalfredprufrock Hey, updates should be published on 0.29.6. Have made another quick update for Record error reporting (was the same maxProperties issue) as well as updated the reporting message as per your suggestion (as the message was inaccurate, so this was a good catch also)

FWIW, I support getting rid of the error messages altogether. They have come in handy in the past, but they could be handled just as easily in userland and as you say, that would open the door for i18n as well.

Yeah, ill take a look at this the next time I get around to making some compiler updates as moving the error reporting into the compiler would be a good opportunity to cull the text messages. Had been considering a @sinclair/typebox/i18n/en-GB|FR-fr etc since writing the error reporting module, although I expect having translations for each language to bump package download sizes considerably (it may actually be a good candidate for an auxiliary package), something to look into in future!

Thanks again!
S

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