pilot icon indicating copy to clipboard operation
pilot copied to clipboard

Make model conform to Hashable

Open danielrhammond opened this issue 6 years ago • 2 comments

Building on the work in #157 we can remove ModelVersion and ModelVersionMixer entirely from Pilot by making model conform to Hashable as part of its requirements.

Pros:

  • Very common that we want Hashable (or at bare minimum Equatable) for our models,
  • Swift 4.2 autogen'ing conformance for simple structs makes this not a pain for most models
  • Makes things feel more swifty (having a separate hasher is definitely a drag in the post 4.2 world)

Cons:

  • Forces Equatable for models (not sure this is actually bad but could be perceived to be higher bar than ModelVersion implementation)
  • Can't separate out hashing for pilots benefit from generic hashing function (this is mostly a good thing, but we exploited this once or twice in $RIP_SECRET_PROJECT to drop noisy user updates for example)
  • Breaking change

I'm solidly pro but wanted to check for feedback before proceeding (cc @wkiefer)

danielrhammond avatar Feb 06 '19 00:02 danielrhammond

Thanks for filing this.

* Can't separate out hashing for pilots benefit from generic hashing function

As far as I can tell, you can still override hashValue and/or func hash(into hasher: inout Hasher). So if you have a case where your server basically calculates this for you as a field, you could use that value directly (or skip hashing fields that don't matter). We just may want to document that.

So yeah, I say the closer to the language the better. 🚀

wkiefer avatar Feb 06 '19 01:02 wkiefer

Oh yeah good clarification, you can totally still override it (and will need to when it can't be auto-gen'd), but what I poorly articulating with that bullet point is that you can't separate out pilots concept of the ModelVersion from the generic hashValue, which means that you can't safely have a less stringent standard for pilot equality without accepting it elsewhere.

This is fine 99% of the time, but we we did exploit making a bad/lossy modelVersion this in the past to make pilot ignore updates from noisy sources, and you likely wouldn't want to make a lossy hashValue to accomplish it since the components considered in hashValue need to also make up the considerations for ==.

I think now that we're using rx more thoroughly it would be expected that you would filter out those updates closer to their noisy source (using distinctUntilChanged) before they even get to the pilot model collection.

danielrhammond avatar Feb 06 '19 17:02 danielrhammond