Conversation
Stypox
left a comment
Stypox
left a comment
There was a problem hiding this comment.
Thank you! Looks good!
An improvement for a separate PR could be to somehow report the source of the data for all skills that rely on external services, right now it's very not transparent.
| - (what does|what is|what s|whats) .word. mean|means? | ||
| - (what is|what s|whats) the? (definition|meaning) (of|for) .word. |
There was a problem hiding this comment.
This is likely not what you meant. | is not like in Regex, where it is the lowest precedence operator. "what is|what s" means either "what is s" or "what what s". See https://github.com/Stypox/dicio-sentences-compiler.
| // Get language code from locale (e.g., "en", "fr", "de") | ||
| val languageCode = ctx.locale.language.lowercase(Locale.getDefault()) |
There was a problem hiding this comment.
It's already lowercase, see https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Locale.html#getLanguage() and the Locale constructors.
| } catch (e: IOException) { | ||
| // Network error | ||
| DefinitionOutput.NetworkError(word = word) |
There was a problem hiding this comment.
Just let this fall through and be handled by the skill evaluator (it will display a nice error message for the network error)
| val definitionData = ConnectionUtils.getPageJson(apiUrl) | ||
| parseDefinitions(word, definitionData) | ||
| } catch (e: FileNotFoundException) { | ||
| // 404 - word not found in Wiktionary |
There was a problem hiding this comment.
What if the $languageCode is not supported? Can the two errors be distinguished? Saying that the definition was not found on any 404 would be wrong.
| val languageCode = ctx.locale.language.lowercase(Locale.getDefault()) | ||
|
|
||
| // Build Wiktionary API URL based on user's locale | ||
| val apiUrl = "https://$languageCode.wiktionary.org/api/rest_v1/page/definition/" + |
There was a problem hiding this comment.
I know there aren't many other tests, but it might be worth writing a test that makes a request to this URL for every possible language supported by Dicio with some known word in that language (e.g. extracted from strings.xml), and checks whether the definition gets resolved correctly.
| } catch (e: JSONException) { | ||
| return DefinitionOutput.ParseError(word = word) | ||
| } |
There was a problem hiding this comment.
Mmmh you have double try-catch. Remove this one and let it be handled in the other try. I'm also wondering whether the JSONException should just be let fall through to the skill evaluator so that it shows a Report button (it should not happen in any case and showing "Could not understand the definition for ..." would not help the user more than showing "An error occurred [throwable message] [report button]".
| } | ||
| } | ||
|
|
||
| private fun parseDefinitions(word: String, data: JSONObject): SkillOutput { |
There was a problem hiding this comment.
Might also be worth writing a test that loads "https://$languageCode.wiktionary.org/api/rest_v1/page/definition/..." and makes sure it is parsed as expected.
2 participants
Another quick skill addition. Uses the Wiktionary API to get word definitions and supports using the user's language to return appropriate definitions. It only reads out the first definition of the word to keep any spoken interaction short.