-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/source description split (agents, spaces) #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Just a few questions for clarification, but nothing holding back merging.
source = models.ForeignKey( | ||
to=Source, | ||
on_delete=models.PROTECT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no CASCADE here? Is there something wrong with deleting associated descriptions when a source is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really an issue on a technical level, but sources are quite fundamental, you can accidentally remove half the database that way. 🙃
return f"{self.person} died c. {self.year_lower}-{self.year_upper}" | ||
|
||
|
||
class PersonReference(Field, models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in assuming you will want to add more properties to this relationship in the future?
(I'm a bit confused why this is not simply a ForeignKey.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's inherits Field
, it has the certainty
and note
properties! I thought those would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, sorry; I missed that one.
|
||
class PersonDateOfBirth(Field, LettercraftDate, models.Model): | ||
person = models.OneToOneField( | ||
to=HistoricalPerson, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused as to why DateOfBirth and DateOfDeath are directly linked to HistoricalPerson, while Gender would be linked to AgentDescription.
I expect dates to be a lot more controversial and source-dependent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't really sure about this, it's true that it's not really consistent.
I think it makes sense to me because I'm not a historian but I have some background in queer theory, so I see dates as boring background information, and representations of gender as dynamic and interesting. That framing depends on research interest, though.
|
||
def clean(self): | ||
if self.location.source != self.agent.source: | ||
raise ValidationError("Can only link descriptions in the same source text") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
This re-does part of #45 - creating a split between historical constructs and descriptions in narrative sources. Refactoring was done to avoid the duplication of every entity having both a Historical and SourceDescription version, which is not always needed.
core
.space
app. These changes are quite minor, mostly adding some fields for where/how information is represented in the source. SpaceDescriptions now require a source - the migrations add a "MISSING SOURCE" object as a placeholder.agent
app, which is more radical. TheAgent
model is split intoHistoricalPerson
andAgentDescription
. I decided to simply delete all existing agents from the database, rather than converting them into one of the new models. For existing agents, we don't actually know if they are described in narrative sources or not.Updates for
letter
andevent
will follow later, but I wanted to avoid a massive PR.Notes:
letter
/event
apps are commented out because they refer to agents (which were deleted).HistoricalPerson
is a single person, while an agent can also be a group. This is because identifying groups as "historical figures" can get ship-of-Theseus-y. It's also less relevant for users.