-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
New exception class that incorporates support for internationalization #2549
Conversation
@cweitkamp for review. Note that the method First solution with getUntranslatedMessage (you need i18nProvider):
Second solution without getUntranslatedMessage (you need i18nProvider and localeProvider):
What is your preference ? |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
4d7e368
to
4e67f90
Compare
In fact, I would be in favor of solution 2, that is removing getUntranslatedMessage. |
It seems like a lot of boilerplate code and it would only work with exceptions this way. Maybe it is also possible to create some kind |
I see String is final and not an interface, probably to prevent tricks like this. 😉 |
But updateStatus is just one use case amongst many. Another common use case is to use e.getMessage() in a log. Ok I can create a LocalizedString type and use it as parameter in the constructor. My current method getUntranslatedMessage() will then return it. And a new version of updateStatus taking a LocalizedString as parameter could be added. But setupI18n() will still be required if you want to use getMessage() or getLocalizedMessage(). |
It would be interesting to know what the other @openhab/core-maintainers think the best approach would be, before you start building something. I recently read that slf4j has localization support, but from the first looks I had at it, it didn't look like it was an easy fit. I also don't know if it works within Karaf, see: |
Please keep in mind that Exceptions and their messages are mostly useful for developers. Localization of these messages might make support more difficult as e.g. I can't understand French or Russian exception messages. |
From my pov we should focus on translatable exceptions here. In detail: a way to use openHAB based way of retrieving a translated exception message via The status message already will be translated by the framework. I am using it in OpenWeatherMap binding in combination with exceptions by passing the Your current |
When I previously said there were many use cases, I was certainly excessive. In fact, we have the use case with updateStatus and the need to translate for the display in UI and we have the other cases where e.getMessage() should return the english message (used in log for example).
The idea is to translate only messages that are displayed in MainUI, in particular the detailed thing status. |
That is what I implemented.
That is the purpose of getUntranslatedMessage to rebuild the "@text/..." key.
Ok, I am changing it. |
bundles/org.openhab.core/src/main/java/org/openhab/core/i18n/I18nException.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
bundles/org.openhab.core/src/main/java/org/openhab/core/i18n/I18nException.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@cweitkamp : all comments handled. Adding unit tests was a good idea ;) I did not add unit tests for the exception created with a throwable cause. If necessary, I can add them. |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
bundles/org.openhab.core/src/main/java/org/openhab/core/i18n/I18nException.java
Outdated
Show resolved
Hide resolved
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.
Sorry, but I still don't think this is a good approach. Exceptions should remain simple information carriers and you should not inject complex objects like Bundle
or TranslationProvider
into them. Another approach could for instance be to have a I18nExceptionFactory
instead that uses the Bundle
and TranslationProvider
to create this exception with an English and localized message.
If you mean a To use a service only on the higher level (thing handler), I imagine more a |
Yes some other approach that keeps it all very developer friendly is also fine with me. As long as it does not inject these complex objects into the exception. The issue with that is that Exceptions can end up anywhere in your application. Then if some code stores these exceptions into a variable, the complex objects cannot be garbage collected once they are no longer used. It would also be nice if you can still create tests and check thrown exceptions without also having to create all sorts of providers/factories. 🙂 |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Ok, I think I have a solution. I am going to create an interface ExceptionTranslationProvider with a method getMessage(exception, bundle, locale) + the corresponding service (I18nProviderImpl). |
Or maybe I could simply add bundle, translationProvider and locale as parameters to the methods getMessage and getLocalizedMessage. It would avoid "injection" In the exception object. |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
This is now changed. |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Any feedback ? |
Yes I see you have addressed my main concern! 👍 Another concern I have with this approach is that it promotes using one meaningless exception type for everything (similar to always throwing That could for instance be solved by making the class abstract so it forces you to think about exception handling and create your own exception types. But this would result in more code and make it less convenient to use. But I think it is good to think this through as it can have consequences when many add-ons start using this. We could also add a few more common exception classes that extend from the abstract class which would make it more easy to use and which also shows how to use it. Many add-ons for instance have to deal with connection or configuration exceptions. I also noticed that it is now a checked exception because it extends from |
I agree that an abstract class would be better. And if it is better to extend |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@wborn : done |
Yes, that is also a good idea. I just want to avoid blocking this PR because we do not agree quickly on a list of exception classes. |
…ption Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Classes Maybe |
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
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 think the PR is in a good shape to get merged now. We can always further tune this in follow-up PRs. I see there is still an unresolved comment by @cweitkamp. If you think it is resolved you can merge this PR (unless you have more comments 😉).
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.
LGTM. Thanks.
@lolodomo May I ask you to write a short abstract for the developer documentation? Thanks. |
…ion (openhab#2549) * New exception class that incorporates support for internationalization * Add ConnectionException, CommunicationException and ConfigurationException Signed-off-by: Laurent Garnier <lg.hc@free.fr> GitOrigin-RevId: 998ce26
Signed-off-by: Laurent Garnier lg.hc@free.fr