-
Notifications
You must be signed in to change notification settings - Fork 11
FEATURE: Add glossary management #46
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
Conversation
@mficzel What is your general opinion on this PR? Should this go to a separate package extending LostInTranslation or would you be willing/interested to include it? |
Did not really look into it as it says it is not Really meant to be merged. At least that is how I understood it. |
In general I would like to have glossaries in lost in translation. Will take deeper look the next days |
From a functional POV, it is ready and we have been using it in production for 1/2 year+. There are some ToDos left in the code that need to be cleanup before merging. Depending on your deeper look, I'd be willing to update the code. |
I agree to the code, it would be really cool to finally have glossay support in here. So feel free to fix the remaining issues. You can ping me via Slack for faster feedback. Otherwise it may take while until i notice movement here. Thanks a lot for your help, it is greatly appreciated. |
5a95cd0
to
cad407a
Compare
protected string $authenticationKey; | ||
|
||
public function __construct( | ||
protected readonly LoggerInterface $logger, |
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.
@mficzel PHPUnit is not happy here:
Error: Cannot initialize readonly property Sitegeist\LostInTranslation\Infrastructure\DeepL\DeepLTranslationService::$serverRequestFactory from scope Mock_AccessibleTestProxyf8c5d1ca5b11a34fe92f72f4fde95814_9b50f72f
How can this be resolved?
4fbcfeb
to
a9828f5
Compare
@mficzel Apart from the failing build, this is now ready for a final review. Most changes I added are related to documentation and configuration. |
a9828f5
to
ffcaaf2
Compare
@lorenzulrich after working on this for a bit i decided to try doing it a little more simple. See #52 for what i ended up with. It would be great if you could test this and give feedback wether it works for you aswell. |
Was implemented here instead #63 thanks a lot for the inspiration to finally give it a shot |
This code is extracted from https://github.com/robert-heinig/Sitegeist.LostInTranslation/tree/feature/glossaries. Since this branch diverged heavily from the original package, I extracted all relevant changes to this branch.
This is not meant from merging as code style-wise the feature is not really in line with the rest of LostInTranslation. However, I'm pushing this as a base for discussion if we should bring this to a state that is mergeable.
This can be configured as follows:
This allows adding a glossary when DE is translated to EN. It relies on the
deeplLanguage
being set in the CR dimension configuration, e.g.:Please be aware of the following:
./flow flow glossary:sync
to set up and sync the glossary (also when installing it).Resolves: #5
cc @robert-heinig