Going through code to try to make sure I understand it completely, as a part of cleaning up Python functions. Using this issue to track possible bugs, refactoring, or questions about the whereaboutsManager class and the get_Whereabouts function. Will open separate issues for other classes.
Bugs/Questions
These are all also commented in code
// shouldn't these just be 0001 and 9999? //
let dateMin = DateManager.normalizeDate('0001-01-01', false)
let dateMax = DateManager.normalizeDate('9999-01-01', true)
// I'm not sure what this is supposed to do //
// origin.startDate doesn't even exist, should this be origin.start? //
if (whereaboutResult.origin && whereaboutResult.origin.startDate) whereaboutResult.origin = undefined
Issue: handling unknown whereabouts
Issue: If I am reading the code correctly, an undefined whereabouts has a different meaning for current/home/origin and for lastknown. For current/home/origin it means "unknown"; for lastknown it can either mean "unknown" if there are no defined whereabouts at all, or "same as current" if there is a defined whereabouts.lastknown but it is the same as current. This is undesirable as any display that wants to report last known whereabouts (e.g., in a table of people last known to be in Tokra) needs to check both lastknown and current and report current if lastknown is empty. Preferable would be to handle the situation where you don't want to print both current and lastknown when you make the display decision.
Issue: There is, in general, no good way to display unknown whereabouts. E.g., if you want to display unknown origins, as far as I can see the only way is to explicitly define { type: home, location: "unknown" } as the first line.
What I'd Like: My preference would be for unknown whereabouts to always be unknown, for ease of use in explicit queries, and that decisions about what to display, specifically be handedly by display code.
Potential Solution/Refactor: I would propose that we, instead of using undefined as shorthand for unknown, explicitly define an unknown normalized whereabout. The simplest option would be to just use whereabouts..location = "unknown", with everything else undefined.
This immediately makes a lot of things simpler. Any function with dataviewjs can trust that you'll always have a current.location, home.location, lastknown.location, and origin.location, which might be unknown. get_Whereabouts can make decisions about what to display based on a) whether locations equal each other, and b) whether locations are unknown. Tables will always show unknown locations which is I think the preferred behavior, but can be filtered if desired.
Then, I'd propose we just bite the bullet and define 16 format strings in metadata.json: for each whereabout type, you'd have a format string for all possible values of known/unknown and current/past. Many of these would presumably be blank.
For people, you might have:
a) Origin format string is blank if origin equals home, but otherwise OriginKnownPast == OriginUnknownPast == OriginKnownCurrent == OriginUnknownCurrent
b) Home format string is as is for KnownPast and KnownCurrent, but blank for all Unknown
c) Current is same as whereaboutsCurrent and whereaboutsPast, no need for a special Unknown version as you'd always display current whereabouts (but for non-people you might want to set the unknown ones to blank unless lastknown != "unknown"
d) Last known same as current but set to blank if lastknown.location == current.location
There might be a bit of fiddly-ness with lastknown as you need a different option if you know the date vs don't.
One additional complication is how to treat pages with no whereabouts: for queries, I want them usually to show up as unknown (e.g, in a table of members of organization x); in headers, probably want this to be blank. But this can be handled by header generation.
getPartyMeeting
This is never used in dynamic code, correct? I would prefer to move this to the header generation code, unless there is a reason why we might explicitly use this code in dynamic elements that should be potentially exportable. There are a few tweaks I might make here as well but need to understand the string formatter first.
Going through code to try to make sure I understand it completely, as a part of cleaning up Python functions. Using this issue to track possible bugs, refactoring, or questions about the whereaboutsManager class and the get_Whereabouts function. Will open separate issues for other classes.
Bugs/Questions
These are all also commented in code
Issue: handling unknown whereabouts
Issue: If I am reading the code correctly, an undefined whereabouts has a different meaning for current/home/origin and for lastknown. For current/home/origin it means "unknown"; for lastknown it can either mean "unknown" if there are no defined whereabouts at all, or "same as current" if there is a defined whereabouts.lastknown but it is the same as current. This is undesirable as any display that wants to report last known whereabouts (e.g., in a table of people last known to be in Tokra) needs to check both lastknown and current and report current if lastknown is empty. Preferable would be to handle the situation where you don't want to print both current and lastknown when you make the display decision.
Issue: There is, in general, no good way to display unknown whereabouts. E.g., if you want to display unknown origins, as far as I can see the only way is to explicitly define { type: home, location: "unknown" } as the first line.
What I'd Like: My preference would be for unknown whereabouts to always be unknown, for ease of use in explicit queries, and that decisions about what to display, specifically be handedly by display code.
Potential Solution/Refactor: I would propose that we, instead of using.location = "unknown", with everything else undefined.
undefinedas shorthand for unknown, explicitly define anunknownnormalized whereabout. The simplest option would be to just use whereabouts.This immediately makes a lot of things simpler. Any function with dataviewjs can trust that you'll always have a current.location, home.location, lastknown.location, and origin.location, which might be unknown. get_Whereabouts can make decisions about what to display based on a) whether locations equal each other, and b) whether locations are unknown. Tables will always show unknown locations which is I think the preferred behavior, but can be filtered if desired.
Then, I'd propose we just bite the bullet and define 16 format strings in metadata.json: for each whereabout type, you'd have a format string for all possible values of known/unknown and current/past. Many of these would presumably be blank.
For people, you might have: a) Origin format string is blank if origin equals home, but otherwise OriginKnownPast == OriginUnknownPast == OriginKnownCurrent == OriginUnknownCurrent b) Home format string is as is for KnownPast and KnownCurrent, but blank for all Unknown c) Current is same as whereaboutsCurrent and whereaboutsPast, no need for a special Unknown version as you'd always display current whereabouts (but for non-people you might want to set the unknown ones to blank unless lastknown != "unknown" d) Last known same as current but set to blank if lastknown.location == current.location
There might be a bit of fiddly-ness with lastknown as you need a different option if you know the date vs don't.
One additional complication is how to treat pages with no whereabouts: for queries, I want them usually to show up as unknown (e.g, in a table of members of organization x); in headers, probably want this to be blank. But this can be handled by header generation.
getPartyMeeting
This is never used in dynamic code, correct? I would prefer to move this to the header generation code, unless there is a reason why we might explicitly use this code in dynamic elements that should be potentially exportable. There are a few tweaks I might make here as well but need to understand the string formatter first.