Skip to content
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

feat: use logger pattern for translator #145

Merged
merged 11 commits into from
Feb 18, 2022

Conversation

lem-onade
Copy link
Contributor

@lem-onade lem-onade commented Feb 2, 2022

Removed old translators (one in dgt-utils and one in dgt-components)

Created new translator with factory

@lem-onade lem-onade marked this pull request as draft February 2, 2022 15:12
@lem-onade lem-onade changed the base branch from master to develop February 2, 2022 15:12
@lem-onade lem-onade linked an issue Feb 2, 2022 that may be closed by this pull request
@lem-onade
Copy link
Contributor Author

Not sure why that test fails, I've tried removing the mocked fetch in tests/setup.ts, but error stays. Seems componentsjs is suddenly looking for npm modules when it shouldn't

@BelgianNoise
Copy link
Member

@lem-onade For some reason test coverage dropped in the dgt-components package.
I am just going to lower it because it doesn't make any sense, good with you?

@lem-onade
Copy link
Contributor Author

@lem-onade For some reason test coverage dropped in the dgt-components package. I am just going to lower it because it doesn't make any sense, good with you?

because there's 1 failing test in dgt-components

@lem-onade lem-onade marked this pull request as ready for review February 3, 2022 09:36
@lem-onade
Copy link
Contributor Author

lem-onade commented Feb 3, 2022

A thought: creating a new MemoryTranslator will always try to fetch the translation file, can we perhaps fetch and store this once in the factory?

or should I not have copied the getTranslatorFor, which returns a translator for a specific class? Perhaps a global translator (with getTranslator) is enough?

@woutermont
Copy link
Contributor

Yes, an (additional) factory that already keeps the files is a good option. Maybe the translator need to be adapted then to take an optional translation file as dependency

@lem-onade
Copy link
Contributor Author

Yes, an (additional) factory that already keeps the files is a good option. Maybe the translator need to be adapted then to take an optional translation file as dependency

Will implement this, also do you that getTranslatorFor and the labels on translators are not needed like they are with the loggers?

@woutermont
Copy link
Contributor

Labels probably not, since nothing is printed (though it would not hurt to leave them in). Still, a getTranslatorFor is usefull to distinguish translators based on different parts of the app (might someday come in handy).

@lem-onade
Copy link
Contributor Author

Labels probably not, since nothing is printed (though it would not hurt to leave them in). Still, a getTranslatorFor is usefull to distinguish translators based on different parts of the app (might someday come in handy).

I refactored getTranslatorFor to return a translator for a specific language, rather than a label, wdyt?

@lem-onade lem-onade enabled auto-merge (squash) February 18, 2022 11:33
@lem-onade lem-onade merged commit 9f4520a into develop Feb 18, 2022
@lem-onade lem-onade deleted the feat/implement-logger-pattern-for-translator branch February 18, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make translator use the same pattern as the logger
4 participants