Deprecate RootObject name

[koeaw]

This PR renames RootObject's name field to deprecated_name, sets it to accept empty string values, and updates references to it throughout the codebase.

Additionally, it swaps Property's duplicated main name fields around:
name_forward does not accept empty strings going forward and is what __str__() gets based on; it's also used to set deprecated_name (instead of the other way around). Property's save() function can now also handle update_fields, specifically updates to any of the 3 name fields.

deprecated_name has a system check message attached to it to make users aware their models need updating. It uses the built-in system_check_deprecated_details attribute, which can/should later get updated to system_check_removed_details (see docs).

PS: The changes resulting in migrations were split out on purpose because Django isn't great at handling field renames and field option changes simultaneously.

[koeaw]

Something I thought of earlier: I'm not 100% sure my simplifiation of the unicodedata.normalize lines in Property's save() method actually work reliably. ... Seems like a classic example for a test case, hmm.

[b1rger]

Can you combine some of the commits? I don't see a reason why a migration should be in a separate commit than the code change itself.
Can you add a scope to the commit messages? (The thing in between the () after the type - releaseplease uses this in the release notes)

The commit message: fix: Property save method - Fix comparisons, comparison order, simplify. does not tell me which problem it tries to solve, especially regarding the PR at hand. Same with refactor: Property help_text options - is it a refactoring of code? And: is the changing of a help_text really that important that we have to run migrations for that? (Maybe it is, the commit message should explain the reasoning)

I think the commit refactor: RootObject field options should be part of the commit that renames the name field to deprecated_name.

[koeaw]

Can reply in more detail later, just quickly for now:

• yes, I can def. always combine code change + resuting migration,
• later, separate changes to field options (+ resulting migrations) are probably just the result of how I did things (in order)... or my wanting to keep essential changes vs. clean-up operations separate so as not to add noise to the former

This refactor took lots of changing around of things and reverting migrations and/or changes in several apps, and I find that keeping everything separate/committing changes incrementally, makes rebasing (shifting around, swapping out things) much, much easier/faster. (... Including while changes are still under review, because it'll help save time in case they need further updating. A lot less relevant after.)

[koeaw]

Can you add a scope to the commit messages? (The thing in between the () after the type - releaseplease uses this in the release notes)

Which app should I best reference when a commit changes several? Always the "originating" one? Or is the "main" one fine as well in cases where a commit is particularly heavy on one app?

E.g. for the breaking changes in the first commit, apis_metainfo because that's where RootObject sits in? But would e.g. apis_relations be ok for commits more focussed on Property?

[b1rger]

Which app should I best reference when a commit changes several? Always the "originating" one? Or is the "main" one fine as well in cases where a commit is particularly heavy on one app?

I don't have a definite answer, but in this case I'd opt for apis_metainfo for the first commit and the other depending on the focus

[koeaw]

Hmm, maybe it would make sense to go by the migrations.

(Posted at the same time as the above comment.)

[koeaw]

Updated the PR as follows:

• merged commit "pairs" for model changes + resulting migrations
• updated commit subjects to add app scope -> picked app in which (main) changes took place
• made commit body for the Property save change more verbose (clarified what was changed)

I can leave the last commit re: Property help_text off. Its intention was to clarify what the fields are used for because those help texts didn't seem very helpful to me. I didn't want to add it to the other field option changes so as not to muddle that earlier commit (but felt it fit with the PR because e.g. the save function also got cleaned up).

[koeaw]

Refactored to integrate overlapping migrations + updated commit messages as discussed elsewhere.

[b1rger]

There is still a reference to .name

File apis_core/apis_relations/models.py", line 272, in get_web_object
    "prop": self.prop.name,
                  ^^^^^^^^^^^^^
AttributeError: 'Property' object has no attribute 'name'

[b1rger]

Ideally, this would have been split up into multiple PRs - the change in apis_core.apis_relations.models.Property from .name to .name_forward could have easily been done in a separate PR in advance. Same for the __str__ methods - if we know, that we will remove them, why first change them and then in the same PR delete them? Why not create a PR first that deletes them? Both those PRs would be a lot smaller to review, they would be a lot easier to test, because there is no change in the datamodel involved...

[koeaw]

Ideally, this would have been split up into multiple PRs - the change in apis_core.apis_relations.models.Property from .name to .name_forward could have easily been done in a separate PR

When looking at things retroactively, maybe. This came up while I was working on this PR. I cannot forsee an "ideal" order of things when those things crop up later.

in a separate PR in advance

Or a later PR.

Either way, it would have meant creating a separate PR that is inextricably linked to this PR (in whichever direction). Which I don't think is ideal in any case, but in my mind will delay things more than having one larger PR. ... See other issues I have worked on or meant to work on, which now linger.

if we know, that we will remove them, why first change them and then in the same PR delete them?

Same as above, this PR reflects the chronology of the work that was done.

I could mash everything together to get rid of that if it makes reviewing easier, but would then have to hand everything over for corrections because I couldn't justify the extra time I'd have to spend on trying to orient myself in the code.

[b1rger]

Either way, it would have meant creating a separate PR that is inextricably linked to this PR (in whichever direction).

If things are inextricably linked, they belong in one commit. I still don't see how the move from .name to .name_forward couldn't have happened in a preparation step- but I'm still working on understanding all the things this PR does, so maybe I'm not seeing something that's clear.

Which I don't think is ideal in any case, but in my mind will delay things more than having one larger PR.

Well, as I've mentioned multiple times before, one large PR is much harder to review than individual smaller ones. I'll can't guarantee I'll have time next week to work on this again, but I'll try my best

[koeaw]

I can split it into two PRs, but likely also won't do this anymore this week.

[vronk]

Can we please prioritize this? we have quite a downstream backlog.
@b1rger could you look at this asap, please, and you agree if this can be merged,
or if @koeaw needs to divide it into two PRs.

[koeaw]

There is still a reference to .name

File apis_core/apis_relations/models.py", line 272, in get_web_object
    "prop": self.prop.name,
                  ^^^^^^^^^^^^^
AttributeError: 'Property' object has no attribute 'name'

It looks like there were two more instances just above that line, self.subj.name and self.obj.name. Got errors for them all, but could still create relationships, it just wouldn't clear the form fields afterwards.

When I changed them, the errors disappeared, but I'm still confused about this. My IDE didn't and doesn't seem to recognise the actual field names, but was happy with the (error-throwing) name references, which it attributed to the built-in set_attributes_from_rel.

[koeaw]

Obsolete due to #707 -> closing.