-
Notifications
You must be signed in to change notification settings - Fork 18
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
Refactor. Translate 2.0 #30
Comments
Things we've discussed:
Other potential considerations:
|
Case in point d7299a7 I've spent a lot of time troubleshooting this and trying to come up with solutions, which in the end turned out just removing error handling there completely. translate/src/translation/translate.js Lines 1683 to 1697 in d7299a7
Before the above change translate/src/translation/translate.js Lines 2352 to 2357 in d7299a7
So the A bug like this shouldn't even exist in the first place if the code was architectured correctly, but even if it did, with proper execution flow figuring out the issue and fixing it should be a matter of minutes, not hours. |
Horrifying! |
After having a brief attempt at uncluttering the messy code in this codebase while trying to fix zotero/zotero#3182, I have decided that it is pointless.
The actual translate flow is actually quite straightforward, but it is massively complicated by everything being super stateful, and execution flow being controlled by tens of flags set externally and internally.
There shouldn't be a need to instantiate any object when translating, instead, you should choose your Translate (Web, Export, Import, Search), then call
translate()
,detect()
(for Web only?) andgetTranslators()
. 99% use cases would simply calltranslate()
. If you pass in no translators intotranslate()
, then it runsdetect()
before translation. If you pass in no translators intodetect()
, then it runsgetTranslators()
before translation.All methods in current translation architecture that begin with
set*()
should become parameters that are passed into the execution calls above.Events/handlers should likely disappear from translate at all. Translate should be strictly concerned with returning metadata from a given translate object. Any further processing like item saving or progress display should be performed by the translate calling code. This is simple to do in practice, since most of the saving functionality is already done in other classes per current architecture, specifically
Translate.ItemSaver
, which all environments that use Translate override.I propose rewriting the translate code from scratch and providing it as a separate file, then gradually exchanging all existing codebases depending on Translate to use the new architecture, and finally phasing out Translate 1.0 after some years and allowing others to switch over.
This would probably be a huge timesaver going forward, since every bug we encounter in translation, and every time we need to change something in the logic usually means a huge amount of time spent analyzing this complex piece of code and further debugging when a change produces unexpected behavior.
CC @dstillman @AbeJellinek @zoe-translates
The text was updated successfully, but these errors were encountered: