-
Notifications
You must be signed in to change notification settings - Fork 330
Refactor validation message generation #910
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
Conversation
a184ea1
to
5904987
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #910 +/- ##
============================================
- Coverage 79.32% 79.08% -0.25%
- Complexity 1224 1252 +28
============================================
Files 123 129 +6
Lines 3904 4044 +140
Branches 739 751 +12
============================================
+ Hits 3097 3198 +101
- Misses 520 554 +34
- Partials 287 292 +5 ☔ View full report in Codecov by Sentry. |
/** | ||
* Functions for working with Locales. | ||
*/ | ||
public class Locales { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read it correctly, this class doesn't work that well with custom MessageSource
s and you need to know what bundle is configured and use the correct list of supported Locales.
What about integrating this functionality to the MessageSource
instead? The source should know what languages it can support and it would be responsible for choosing the Locale
.
Then in the ExecutionContext
/ExecutionConfig
, you'd set only the priority list and it would get resolved through the MessageSource
automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to separate the concerns since the use of a priority list is really an optional thing.
Attempting to implement it in the MessageSource
means that that it needs to keep evaluating the priority list each time for each message which won't be efficient.
If you have a custom MessageSource
then you would know what locales you support. Then you can just use Locales.findSupported(priorityList, mySupportedLocalesForMyMessageSource)
and just use that Locale to set in the ExecutionConfig
.
Also you might come from a framework that already implements some of these parts, for instance Spring and then you can just use those instead, for instance Spring has a LocaleResolver and MessageSource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense to leave the locale resolution on the client. The resolution has to be done in a bigger context (let's say of the whole app), so that the translation is consistent.
But I still think having the list of supported languages would be better nicely coupled inside the MessageSource
.
I think it's not ideal to rely on the fact that the developer knows what is being used where, it's better if there's a possibility to can get the information through a single API when needed. And it shouldn't matter if you use the default bundle or a custom bundle, there should be one API for one task.
E.g. if this library becomes a part of something more complex, I can imaging some piece of a (utility) code that prepares some ready-made JsonSchema
with some custom bundle. And a different piece of code for interoperability between this library and some other framework (like Spring classes that you mentioned), let's say this code is there to choose the correct locale in you app and automatically set the best candidate for your json schema validation.
These pieces can be coming from different libraries/developers and ideally, they should work together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following is the typical usage
SchemaValidatorsConfig config = new SchemaValidatorsConfig();
JsonSchemaFactory factory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V201909);
JsonSchema schema = factory.getSchema(source, config);
String accept = "it;q=0.9,fr;q=1.0";
Locale locale = Locales.findSupported(accept);
ExecutionContext executionContext = jsonSchema.createExecutionContext();
executionContext.getExecutionConfig().setLocale(locale);
Set<ValidationMessage> messages = jsonSchema.validate(executionContext, rootNode);
You can always build your own abstraction on top of this if you want to prepare a read-made JsonSchema
with some specific configuration and some custom bundle to hide the implementation details. This will also let you switch out to a different json schema validator if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand that the usage, I've seen it in the new documentation, I was just suggesting to put the list of locales to the MessageSource, because it can be considered a property of a message source. Of course, one can make abstractions without that, but IMO it would be more convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justin-tay Just want to confirm with you if this change is a breaking change. If yes, we need to make sure that we increase at least a minor number for the next release. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this and in particular the changes in subsequent pull requests I have made have breaking changes. Thanks for your effort in maintaining this project, it's much appreciated!
5904987
to
09c24d6
Compare
09c24d6
to
c4840b3
Compare
Resolves #908, resolves #903, resolves #839, resolves #803, resolves #744
This makes the following changes
MessageSource
interface for deriving localised messagesExecutionConfig
to storeLocale
and use it for per-execution localised messagesLocales
to determine the supported language tags for the default resource bundle and for determining the best matchingLocale
MessageFormat
for formatting custom messages with arguments