Add LazyEncodingRegistry #24
Conversation
A lazy initialization implementation of EncodingRegistry. It does not register any encoding until either the getEncoding(EncodingType) or getEncoding(String) method is called. When one of these methods is called, the requested EncodingType is registered.
There was a problem hiding this comment.
Thanks for tackling this issue :) I think there are some minor problems that we need to get fixed before merging this
There are some more details to it, than I thought. I commented them inline. It would be great, if you could also write tests for the new LazyEncodingRegistry to ensure that the lazy loading works for every case and no regressions creep in
| @@ -10,12 +10,21 @@ public final class Encodings { | |||
| * | |||
| * @return the new {@link EncodingRegistry} | |||
| */ | |||
| public static EncodingRegistry newDefaultEncodingRegistry() { | |||
| final DefaultEncodingRegistry registry = new DefaultEncodingRegistry(); | |||
| public static EncodingRegistry newEagerEncodingRegistry() { | |||
There was a problem hiding this comment.
I think we should keep the newDefaultEncodingRegistry naming here in the API. While your naming is more expressive, it will break every consumer of this library on upgrade, which I would like to avoid
There was a problem hiding this comment.
Ah, I forgot that this is a library. I'll change the name back to its original one.
|
|
||
| @Override | ||
| public Optional<Encoding> getEncoding(String encodingName) { | ||
| addEncoding(EncodingType.valueOf(encodingName)); |
There was a problem hiding this comment.
The getEncoding(String) method is the only method that registered custom encodings can be retrieved with. In this current implementation, it will throw, if encodingName does not correspond to a pre-defined encoding. This leads to the LazyEncodingRegistry being unusable with custom encodings, which is not what we want 😄
The flow of this method should be:
- Check if a encoding for that string is registered (if yes, return it)
- If not, check if the encodingName matches a pre-defined encoding
- If yes, re-use the lazy initialization flow of the getEncoding(EncodingType) method
- It not, return an empty optional
Prevent from loading an encoding multiple times.
This method is for getting an EncodingType from the encoding type name.
- Create LazyEncodingRegistryTest for testing LazyEncodingRegistry. - Refactor tests, create BaseEncodingRegistryTest for reusing tests for LazyEncodingRegistryTest and DefaultEncodingRegistryTest.
|
Hi, @tox-p, in addition to the suggestions you provided, I have also created a new |
| return encoding; | ||
| } | ||
|
|
||
| addEncoding(EncodingType.fromName(encodingName).orElseThrow(() -> new IllegalArgumentException("Unknown encoding type: " + encodingName))); |
There was a problem hiding this comment.
This should not throw but instead return an empty optional to keep the method contract ("if there is no custom encoding with that name and there is no predefined encoding with that name, return optional#empty")
To prevent further roundtrips, I'm gonna make this little change after merge :)
A lazy initialization implementation of EncodingRegistry. It does not register any encoding until either the getEncoding(EncodingType) or getEncoding(String) method is called.
When one of these methods is called, the requested EncodingType is registered.