search

tterrag1098 / Registrate

Your mod's best friend - keep your registry objects simple and organized
Mozilla Public License 2.0
120 stars 33 forks source link

`.lang()` methods considered harmful #38

Open T3sT3ro opened 3 years ago

T3sT3ro commented 3 years ago

I found several reasons why .lang() methods should be removed, and maybe replaced with some other, decoupled and centralized way to manage localization. So let me explain why:

They make it harder to resolve git conflicts

Recently, I wanted to rebase a branch on top of master. It was basically a medium-sized update with partial rewrite of some classes, so not a simple no-brainer this time.

Then, the problem emerged:

This problem shouldn't even be present in the first place, but because both commits touched the same file it couldn't be resolved automatically. Because the .lang() method exists, we can unintentionally introduce git conflicts on the i18n<--->logic boundary. It's not something you think about, until you have this problem and you have to resolve the conflicts manually.

I'm sure you know that the convention of hardcoding strings is considered bad practice. Proper i18n should have it's strings stored in a .lang/.string.xml or similar file -- they act as a centralized "dictionary" that is easy to find, edit and maintain.

These conflicts require developers to resolve the language problems manually, but we can't/don't want to make decisions about language when we're just changing code. Often solving conflicts boils down to "accepting theirs" on language and "accepting ours" on code, but everyone can make a mistake and produce a faulty commit (either we skip a better translation or reject valid code due to missclick).

Another thing...

...keys are mostly immutable, while translations and code may change frequently

A thing once added will keep it's translation key mostly forever/until renamed/removed, so we can think of them as mostly immutable. Translation strings on the other hand can get sudden bursts of git activity even several times a day. Code changes can happen daily. Having to resolve the conflicts by hand can add otherwise redundant overhead to workflow.

Yet another thing -- it's hard to localize strings when they are scattered all over the codebase.

Some objects will use .lang(), others won't. Without centralized place to manage translations it's becoming hard to locate correct place for strings.

I've seen at least one example, where developers write some custom language managing code

They make it harder to integrate translation service, e.g. Weblate

No centralized spot to manage all the translations

Sum up

IMO .lang() methods encourage bad practices and make it harder to work with. In conclusion - I would get rid of them. What do you think?

tterrag1098 commented 2 years ago

I agree that the pollution of business logic (and the merging pain that comes with it) is an issue, but not with the rest. Nothing about these methods prevents localization of english strings, in fact it encourages it by forcing use of datagen to create standard lang .json files.

I think you're getting confused by that code in Create, it's only for datagen. Any translation service would be operating on the json files directly, which are stored as normal in the repository.

I am open to solutions of how to improve the separation of concerns here, though personally I have not found it to be much of an issue -- the calls to .lang() will generally be on their own line of code in the builder chain. Additionally, in my use cases, the automatically generated english name is fine about 90% of the time, so there's not even any code at all.