Conversation
|
While continuing with cli translation I encountered some import cycle problems, mainly caused by Config using utils. Since the language part will be included almost everywhere I moved it to a separate package to reduce the imports to a minimum. To remove the Config - Utils dependency I moved the two functions to the Config module since they were not used anywhere else. But I do not know whether this is a good and acceptable solution. Recommendations for better fixes welcome 🤗 |
|
The last commit is an early commit, so you can review all the changes, although the cli translation is not complete yet. Since I encountered more import cycle problems, I have removed the config dependency from lang, so to enforce a certain language, you now need to use a env var instead of a config entry. But I guess, it is best to keep the language part as dependency-free as possible and the env var is a suitable alternative. If you want to change the language permanently, you can easily add an alias or a small wrapper script. Or do you perhaps have a better idea? Will need to think about how to do this in the backend/frontend. Perhaps a settings in the user profile, but that is something to think about when cli is done. |
benbusby
left a comment
There was a problem hiding this comment.
Hi @NaLiJa, I just finished a quick review of your progress so far. It's looking good, thank you for all of your effort so far!
I'd like to keep the cli/utils package as dependency free as possible so that we don't have to worry about import cycle errors moving forward. On that note, I've made a few suggestions for how to remove the lang dependencies from the utils package so that there shouldn't be any import errors. Hopefully these make sense, let me know if I need to clarify anything.
I also made some changes to the translation key names that are more descriptive and/or easier to read quickly.
I do think the language configuration should happen in the config.yml file so that we're not splitting up CLI configuration into different paradigms. But with the removal of the lang dependency from the utils package, that should be possible again hopefully? Let me know if you're still having issues and I'll take another look at ways to decouple some of the packages.
cli/utils/utils.go
Outdated
|
|
||
| if len(splitTag) != 2 { | ||
| return "", nil, errors.New("invalid download string") | ||
| return "", nil, errors.New(lang.I18n.T("cli.utils.error.invaliddl")) |
There was a problem hiding this comment.
This whole function (ParseDownloadString) is actually leftover code that can be removed -- the URL parsing for download links is now handled in cli/commands/downloads/downloads.go -> parseLink(link string). That should reduce a need for importing the lang package.
There was a problem hiding this comment.
It is still used in util_test.go, it seems. Shall we move it there?
There was a problem hiding this comment.
You can remove the test case for it as well since it's not being used
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
Co-authored-by: Ben Busby <contact@benbusby.com>
…into feature/locales
|
Thanks for reviewing! I applied your suggestions and hopefully caught all the necessary code changes. The import cycles are still a problem, mainly caused by "lang" now needing "config" and "config" needling "lang". I have pushed the current state with the import cycle, so you can see the problem. A possibility would be creating a stripped down copy of config that is only capable of reading the config file - which would be not so nice from the software structure point of view - or we try to split the config package into a pure config reader and a managing part and keep the reading part free from translation - if that separation is possible without major impacts on the rest of the project. What do you think? |
|
Hmm, let me think about that and take another look. I should have time later today to review the import cycle between lang and config and see if there's a particular path forward I think we should take. |
|
I have tried to fix this with splitting config into the file handling part and the rest. This is in a separate branch https://github.com/NaLiJa/yeetfile/tree/feature/splitconfig |
|
Hi Ben, sorry, I just noticed that ... again ... my last commit to the splitconfig branch was not complete. The configbase was missing. Sorry, don't know why I had a * in my .gitignore there. SORRY! |
|
Hi @NaLiJa, sorry for disappearing for a while. I got really busy with contracting work and haven't been feeling like working on YeetFile. The overall interest in the project was a lot lower than I anticipated, so the effort to reward ratio has been pretty low for me. I really appreciate the effort you've put into the client side translations though and will get around to reviewing this eventually if you're still interested in working on it. My plan for the immediate future is to work on scaling down the official yeetfile.com instance and then eventually getting back to implementing new features, such as this multi-language feature. |
|
Hi @benbusby no problem, of course! I have been in the same boat in the last two weeks, May is always overloaded with work, but it should get better end of next week. I definitely want to continue with the translations and took some first step to add this to the template engine, but it is incomplete, so I didn't push it yet. IMHO yeetfile is a really great project and I hope to start using it after the summer for our school. BTW, yeetfile was added to yunohost recently, so maybe this gives some boost... |
This is the start for adding multi language support. It is very much WIP. This PR is meant to track the ongoing development.
This adds: