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: add ability to set a custom verbose name #770

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

coreyar
Copy link

@coreyar coreyar commented Dec 4, 2024

Adds the option to override the verbose name created by DjangoModelTranslation to the TranslationOptions.

This allows for creating a custom verbose name to be displayed in the Django Admin.

Note
I tried to run linters on the project and it resulted in a huge set of changes so I left them out of this PR.

Closes #768

@last-partizan
Copy link
Collaborator

Let's continue discussion here.

I took a look and I think that adding it to the TranslationOption makes the most sense.

The default method would be

def get_verbose_name(self, verbose_name, language):
        return build_localized_verbose_name(verbose_name, language)

and then the change to the TranslationField

- self.verbose_name = build_localized_verbose_name(translated_field.verbose_name, language)
+ self.verbose_name = trans_opts.get_verbose_name(translated_field.verbose_name, language)

The other place this affects is in build_localized_intermediary_model where I can add getting the options to access the correct function for getting the verbose_name. Maybe we need a second for the plural verbose name.

I think TranslationOptions makes the most sense and is also logical for updating the documentation.

From my point of view, TranslationOptions is supposed to be model-related. And when you do it this way, you need to subclass TranslationOptions, and use your new class for every case when you want this new labels.

Maybe it's better to use approach with adding new setting? What benefits of current approach with TranslationOptions over my suggestion with settings?

(That's not a request to rewrite, just a discussion. You're building this feature for yourself and it's fine either way)

@coreyar
Copy link
Author

coreyar commented Dec 5, 2024

Sure, My understanding is that verbose_name is part of the model. It is documented under model fields in the django docs. We might say that technically it is part of the field but we have some field level model settings in TranslationOptions currently.

The goal is to customize the verbose_name where it is accessed on the model.field so I'm not concerned or think it would be appropriate to subclass TranslationOptions and use this method in other places.

I like a solution that keeps all code for translating models in the translation.py. It doesn't feel intuitive to have to reach for settings.py to setup a custom verbose_name function when call the code related to this is in the models.py and translation.py files.

The approach of using TranslationOptions is also nice because you could have different name formatting for different models.

@last-partizan
Copy link
Collaborator

The other place this affects is in build_localized_intermediary_model where I can add getting the options to access the correct function for getting the verbose_name. Maybe we need a second for the plural verbose name.

Only if you plan to use it. If not, we can leave default here.

The approach of using TranslationOptions is also nice because you could have different name formatting for different models.

Do you really need this? From my point of view, it would be simpler to just change setting, and have all models use new format. Also, variant with a setting takes care of build_localized_intermediary_model automatically.

@coreyar
Copy link
Author

coreyar commented Dec 6, 2024

The approach of using TranslationOptions is also nice because you could have different name formatting for different models.

Do you really need this? From my point of view, it would be simpler to just change setting, and have all models use new format. Also, variant with a setting takes care of build_localized_intermediary_model automatically.

No, I doubt there is much of a practical need for it. I see what you are saying with build_localized_intermediary_model and having a global formatting setting.

@last-partizan
Copy link
Collaborator

Great, let's remove that last obsolete test and we can merge this.

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.

Customize field label
2 participants