acdh-oeaw / apis-core-rdf

APIS Core refactored
https://acdh-oeaw.github.io/apis-core-rdf/
MIT License
3 stars 3 forks source link

Replace `title()` uses on entity classes #77

Closed SteffRhes closed 3 months ago

SteffRhes commented 1 year ago

Throughout the codebase there is legacy use of title() on entity class names. All of them need to replaced. Also the usage of it must be avoided in the future.

It was be used in two contexts where for both of them are problematic:

I can't think of any other use-case than those two. And since we should not use it in those two, we should not use title() on entity class names at all anymore.

Pycharm tells me of 44 occurrences of title(), excluding commented code.

koeaw commented 1 year ago

Almost. As said elsewhere, there's a point to .title() when it's used on verbose names of classes, since those are assumed to be lower case.

SteffRhes commented 1 year ago

title() is used by Django on verbose names, yes, internally. But we are talking here about replacing our own usage of it, which should be dropped.

koeaw commented 1 year ago

No, I was specifically talking about our own use, used by e.g. me here, which is only conveniently clear because I chose an unambiguous variable name.

My comment was targeted at whoever ends up picking up this issue so they don't take this:

All of them need to replaced. Also the usage of it must be avoided in the future.

at face value because it could lead to them accidentally removing those valid/useful cases of .title()-ing on verbose names as well.

SteffRhes commented 1 year ago

It's better to set verbose_name manually for classes like so:

class E40_Legal_Body(E1_Crm_Entity):
    class Meta:
        verbose_name = "Legal Body"

This way we can define within the ontology one precise label to be displayed for humans, and we can separate it from the technical model class name.

Hence, no need for title(). Just use model._meta.verbose_name alone.

koeaw commented 1 year ago

This is already happening.

As said elsewhere, there's a point to .title() when it's used on verbose names of classes, since those are assumed to be lower case.

b1rger commented 3 months ago

After #1015 there will only be one occurrence of title() left, namely: https://github.com/acdh-oeaw/apis-core-rdf/blob/d8649d7d907d07f2ef6b3e0def374d52645d46cc/apis_core/apis_relations/tables.py#L159

This one will be either dropped by refactoring the file or by replacing the whole temptriple approach.

So I'm closing this issue