Spanish dictionary support #47
Conversation
ArtemSkrebkov
force-pushed
the
askrebko/spanish_dictionary
branch
from
1fe52e0
to
e8b34a6
Compare
There was a problem hiding this comment.
Thanks for submitting this PR!
I'm not opposed to this reorganization but am curious if it is necessary. There's a more-specific comment in these notes, but the macro question is, what would the code look like for addDefinition, addPronunciation etc if you used the xml API? Could you simply re-use most of the existing parsing code?
| myPath = os.path.dirname(os.path.abspath(__file__)) | ||
| sys.path.insert(0, myPath + '/libs') |
There was a problem hiding this comment.
any reason to have this up here? would prefer not to mix imports & code
| if settings.PREFERRED_DICTIONARY == "COLLEGIATE" or settings.PREFERRED_DICTIONARY == "MEDICAL": | ||
| cardBuilder = cardbuilder.CollegiateCardBuilder(word) | ||
| elif settings.PREFERRED_DICTIONARY == "SPANISH": | ||
| cardBuilder = cardbuilder.SpanishCardBuilder(word) |
There was a problem hiding this comment.
let's add an else case here and raise or print an error message -- it looks like validate_settings may just allow continued execution
| myPath = os.path.dirname(os.path.abspath(__file__)) | ||
| sys.path.insert(0, myPath + '/../') | ||
| sys.path.insert(0, myPath + '/../libs') |
| if not settings.PREFERRED_DICTIONARY in ["COLLEGIATE", "MEDICAL", "SPANISH"]: | ||
| message = "Setting PREFERRED_DICTIONARY must be set to COLLEGIATE, MEDICAL or SPANISH. Current setting: '%s'" \ | ||
| % settings.PREFERRED_DICTIONARY |
There was a problem hiding this comment.
can we pull out ["COLLEGIATE", "MEDICAL", "SPANISH"] into a set as a global variable somewhere? Then we can re-use it in the message and potentially refer to it elsewhere in code as well
| def getCard(self) -> Card: | ||
| return self._card | ||
|
|
||
| class CollegiateCardBuilder(CardBuilder): |
There was a problem hiding this comment.
Looks like this is functioning as a card builder for both medical as well as collegiate? Let's rename this as GenericCardBuilder or something and then subclass Collegiate and Medical from it (even if their bodies will, for now, be empty).
|
|
||
| self._card.fields[settings.PRONUNCIATION_FIELD] = to_print | ||
|
|
||
| class SpanishCardBuilder(CardBuilder): |
There was a problem hiding this comment.
It looks like we're using the JSON api here instead of the xml one -- I am assuming that's why there is much less code re-use between the two. Can you describe why to make this choice? I'm concerned this is duplicating a significant amount of parsing code, which can really easily start to drift. Did you try to simply re-use the existing code to parse the xml-based responses? If we want to wholesale switch to JSON, that is another possibility. But trying to consume both XML+JSON response types from the same API is not ideal.
There was a problem hiding this comment.
Thanks for the reply!
The thight is that XML API is deprecated and there is no way to get a key for it. At least, I didn't find one. If you know how to get a key for XML version of Spanish dictionary, please share, I will definitely give it a try.
I want to extend AutoDefine to work with Spanish language and the fastest way (rather the only :)) I came up with is to implement a JSON parser for merriam webster dictionary API. I guess that the parser I implemented in SpanishCardBuilder should work with other dictionaries but I need to do more testing to confirm it.
There was a problem hiding this comment.
Yeah I don't mean to block you too much but with this large of a refactoring I would prefer if we switch all the code over to the same base parsing code, ideally JSON. Happy to review that if you add commits to this PR!
There was a problem hiding this comment.
It is not a problem. I am glad to help if it makes the addon better.
Does it make sense to keep XML as an option? We can say that XML is deprecated and will be removed in future.
It might be useful if JSON parser introduces some bug.
In that case, a user can switch back to XML instead of re-installing an older version of the extention.
There was a problem hiding this comment.
My bias would generally be to remove that support altogether as I think the option will almost never be used by users & will mean a higher volume of code to maintain. If M-W decides to actually break the JSON API for a long period of time, we should just push out a full add-on update to go back to XML. We can always find that code in the git history. But if you find a simple way to keep it we can do so -- I'll let you make the call.
Hello @z1lc
Thanks for a useful addon!
I added support for spanish dictionary. I think it might be useful for others.
Please take a look at the approach suggested. I think it can help to make AutoDefine a bit easier to extend.
If you are OK with it, I can add an instruction how to add a new dictionary to the addon.
Hope you will have time to review the PR.
Thanks!