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

Make model conform to Hashable #158

Open
danielrhammond opened this issue Feb 6, 2019 · 2 comments
Open

Make model conform to Hashable #158

danielrhammond opened this issue Feb 6, 2019 · 2 comments

Comments

@danielrhammond
Copy link
Contributor

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)

@wkiefer
Copy link
Contributor

wkiefer commented Feb 6, 2019

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. 🚀

@danielrhammond
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants