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.

Sign up for GitHub

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

Jump to bottom

Suppress nil patch events in QueryT as an optimization #203

Merged
merged 5 commits into from
Mar 22, 2019
Merged

Suppress nil patch events in QueryT as an optimization #203

merged 5 commits into from
Mar 22, 2019

Conversation

kmicklas
Copy link
Contributor

Note that this now requires the query type to have an Eq instance.

Suppress nil patch events in QueryT as an optimization
538e879 
Note that this now requires the query type to have an `Eq` instance.
@kmicklas kmicklas requested review from a user and ryantrinkle July 18, 2018 19:41
ghost
ghost reviewed Jul 18, 2018
View reviewed changes
src/Reflex/Query/Base.hs Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jul 18, 2018

It was actually a question whether it was legit, not a request.

@kmicklas
Copy link
Contributor Author

I think it's pretty clearly legit semantically either way. I left it out initially because I figured those ones would be not mempty frequent enough to make the comparison not worth it. But this is totally baseless guessing.

@hSloan hSloan added the follow up Follow up with requester to take some action label Jan 23, 2019
@ali-abrar
Copy link
Member

Do we have a perf test case for this?

@kmicklas
Copy link
Contributor Author

@ali-abrar No, we don't.

@Ericson2314
Copy link
Member

  1. Add NonEmpty containers, without imbalancing or massive code duplication haskell/containers#608 (comment) the Non Empty changes are actually look quite performant so far, so we might eventually want a maskMempty :: Something a b => a -> Maybe b

  2. That Something need not require all of Eq, For example, with anything map-like can use null from Foldable. Extending our patch class hierarchy to have an isEmpty (and maskEmpty if we wish to be able to have associated non-empty type per the above) can make this work.

@ali-abrar
Copy link
Member

@kmicklas I'd like to include this in the 0.6 hackage release. Can you add a note to ChangeLog.md since this is a breaking change?

@ali-abrar
Merge remote-tracking branch 'origin/develop' into queryt-optimization
da669e3 
* origin/develop: (52 commits)
  Rename MonadDynamicWriter
  Add changelog
  Add Num instance for Dynamic
  Bump to version 0.6
  Re-export mapMaybe from Filterable
  Update Collection.hs
  Remove some more long-deprecated functions
  Organize exports under correct headings
  Fix warnings
  Deprecate FunctorMaybe in favor of Filterable
  Unorphan MonoidalMap Group and Additive instances
  Remove some long-deprecated functions
  Add some docs
  Add matchResponsesWithRequests
  Add withRequesterT
  Bump version in default.nix
  Bump version to 0.5.0.1
  Relax bounds on 'these', add explicit import to avoid clash
  Update the types of merge and fan
  Add missing haddock hyperlinks
  ...
@ali-abrar
Update changelog for #203
f40a0f4
@ali-abrar ali-abrar merged commit 86b821a into develop Mar 22, 2019
@kmicklas kmicklas deleted the queryt-optimization branch March 22, 2019 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryantrinkle ryantrinkle Awaiting requested review from ryantrinkle

Assignees
No one assigned
Labels
follow up Follow up with requester to take some action
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kmicklas @ali-abrar @Ericson2314 @hSloan @luigy