Deduplicate the make_icu_data method in python#75642
Deduplicate the make_icu_data method in python#75642bradc6 wants to merge 1 commit intogodotengine:masterfrom
Conversation
|
I understand the wish to reduce code duplication, but given that it's a pretty simple function, I'm not convinced that this is worth the trouble. It makes more sense to me for this code to stay within the modules which use it, instead of making it global. |
My concern would be that it is easy for them to get out of sync since it's possible for someone to expand a feature or fix a bug in one but miss the others. Maintainers/contributors need to ensure that all instances are up to date and they may not be familiar or aware of the duplication. We can imagine one of them getting out of date and could cause implicit and inconsistent build behavior. Maybe there is a better shared python file location/instance; but ultimately it's up you whether it's worth it or not. I'm not directly blocked by this but noticed it while working through some code |
bruvzg
left a comment
bruvzg
left a comment
There was a problem hiding this comment.
It's a tiny method, specific to the module, and most likely it will never change, so I'm not sure if it worth it.
| self.Append(CXXFLAGS=["-w"]) | ||
|
|
||
|
|
||
| def make_icu_data(target, source, env): |
There was a problem hiding this comment.
This one is completely redundant (fallback TS do not use ICU) and can be removed.
| self.Append(CXXFLAGS=["-w"]) | ||
|
|
||
|
|
||
| def make_icu_data(target, source, env): |
There was a problem hiding this comment.
Won't work, this copy is for the GDExtension build, which does not include global methods, so Sconstruct in the gdextension_build should have code similar to the changes in SCsub to load it. And I'm not sure it won't conflict with other GDExtension stuff.
There was a problem hiding this comment.
What would you recommend is the best way to run into this error?
There was a problem hiding this comment.
You can run GDExtension build by cloning godot-cpp into modules/text_server_adv/gdextension_build and running scons from modules/text_server_adv/gdextension_build.
3 participants
There are multiple copies of the same function and this commit attempts to merge them together. I'm not super excited about the sys,path manipulation so if there is a better way please let me know. :)
Thank you for reviewing