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

Figure out what I really mean by StewardData #46

Closed
elidupree opened this issue Sep 6, 2017 · 3 comments
Closed

Figure out what I really mean by StewardData #46

elidupree opened this issue Sep 6, 2017 · 3 comments

Comments

@elidupree
Copy link
Owner

Currently, various parts of the code require trait StewardData, but I'm not sure they have the same actual requirements, or if the requirements I've chosen are exactly the correct ones.

Also, if/when StewardData is actually the correct concept, if it's just a collection of supertraits, I probably want to make a blanket impl so that you don't have to implement it yourself all over the place.

@elidupree
Copy link
Owner Author

Current places StewardData is required:

  1. For the base time type
  2. For Event types
  3. Inside DataHandle, and Globals; and because of this, DataHandle, EventHandle, and DataTimelineCell themselves must implement it
  4. For queries and query results, and therefore SimpleTimeline data

Also, DataTimeline itself DOESN'T require StewardData as a supertrait, which is worth reconsidering.

And the supertraits required by StewardData:

  • Any (I'm not worried about this one)
  • Clone. Surprisingly, this is not currently very required for base times – we only explicitly clone them while mucking about with ValidSince and other API stuff, and I think it could all be done by requiring the user to pass in Time instead of &Time. However, there's no reason NOT to require Clone for Time. Queries definitely want Clone so that values can be stored for auditing, although that might be modified somehow by query-by-reference. DataTimelineCell actually wants to NOT implement Clone, because cloning them is weird about history, so DataHandle innards and Globals want to not require it as well. DataTimeline requires Clone at the moment, although I don't think we really want to be cloning it except maybe for auditing. We never clone Events, and there's not much need to clone them for auditing because they're immutable.
  • Eq. This requirement is irrelevant for Time because Time has to be Ord anyway. Queries need it for auditing purposes. It would be weird on DataTimeline, and not helpful for auditing because differing speculative futures are permitted. Implementing it for DataHandle innards MIGHT have a use for auditing, but it's weird because they can contain DataTimelineCells. (Currently, DataTimelineCell does Eq by unique id, so Eq between 2 different objects containing them would always be false.) Event should probably follow along with the other simulation data types.
  • Serialize (+ DeserializeOwned): Time, Globals, Event, DataTimeline, and DataHandle innards obviously need this for actual serialization. Queries don't necessarily need it, although I think I should require it for future-proofing in case I want to share queries across a network for auditing purposes.
  • Debug (I'm not worried about this one)

Also, PersistentlyIdentifiedType ISN'T required by StewardData, which is worth reconsidering. Time and Globals don't need it, although I might want to stick Globals behind a DataHandle, which would require it. Events and direct innards of DataHandle require it for serialization. Queries and query results will need it if they want to be able to use Serialize.

In summary (including only the disputed traits; ! means "definitely not required", not "required to not have it"; DeserializeOwned is automatically included with Serialize):

Time:
 Clone + ?Eq +  Serialize + !PersistentlyIdentifiedType
Event:
?Clone + !Eq +  Serialize +  PersistentlyIdentifiedType
DataTimeline: 
?Clone + !Eq +  Serialize + !PersistentlyIdentifiedType
Globals:
!Clone + !Eq +  Serialize + ?PersistentlyIdentifiedType
DataHandle generic parameter:
!Clone + !Eq +  Serialize +  PersistentlyIdentifiedType
Other stuff often used inside DataHandle:
!Clone + !Eq +  Serialize + !PersistentlyIdentifiedType
Queries and query results:
 Clone +  Eq + ?Serialize + ?PersistentlyIdentifiedType

@elidupree
Copy link
Owner Author

elidupree commented Sep 6, 2017

In conclusion, I don't see any real pattern here. There is no call for a single "StewardData" trait to unite all of these uses.

HOWEVER, we do need some form of abbreviation – it's pretty bad to have a long list of traits that's repeated in a lot of places, especially if you eventually decide you need to add or remove a trait from the list. So I propose that each long list of traits be abbreviated into a single trait, with a blanket impl.

trait BaseTime: Any + Debug + Send + Sync + Clone + Ord + Serialize + DeserializeOwned + Hash {}
trait SimulationStateData: Any + Debug + Serialize + DeserializeOwned {}
trait Query:       Any + Debug + Clone + Eq + Serialize + DeserializeOwned + PersistentlyIdentifiedType {}
trait QueryResult: Any + Debug + Clone + Eq + Serialize + DeserializeOwned + PersistentlyIdentifiedType {}

DataTimeline and Event are already traits, so their lists of supertraits already don't need to be repeated everywhere.

@elidupree
Copy link
Owner Author

Note: normally, Serialize + DeserializeOwned + !Clone types are a little weird because serializing followed by deserializing is a de facto clone. However, in this case, the Serialize impls aren't actually available in all cases – DataTimelineCell and the handles panic if you try to serialize them in any context other than the "serialize the whole simulation state" context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant