Leading vs. trailing edges for Control.Debounce

[thomasjm]

Okay @snoyberg , took a stab at #745 . What do you think?

• Bumped the version number
• Documented new APIs with Haddock markup
• Added @since declarations to the Haddock

After submitting your PR:

• Update the Changelog.md file with a link to your PR
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

[thomasjm]

Oh, I realized that this doesn't actually break the public API, so maybe the version should just bump to 0.1.6 instead of 0.2.0?

[snoyberg]

Yes, I believe that's the right version number. Making that change should allow CI to pass.

[snoyberg]

Looks like there's a build failure with older versions of GHC:

auto-update         > /home/vsts/work/1/s/auto-update/test/Control/DebounceSpec.hs:9:8:
auto-update         >     Could not find module ‘Control.Monad.IO.Class’
auto-update         >     It is a member of the hidden package ‘transformers-0.4.2.0@trans_GZTjP9K5WFq01xC9BAGQpF’.
auto-update         >     Perhaps you need to add ‘transformers’ to the build-depends in your .cabal file.
auto-update         >     Use -v to see a list of the files searched for.

[thomasjm]

The remaining failures seem to be related to warp

[kazu-yamamoto]

Very strange. tls 1.4.1 surely exports CompressionID.

[kazu-yamamoto]

@thomasjm Would you do git rebase master and git push -f?

[thomasjm]

Slightly different failures now, but still warp spec and warp-tls build respectively

[kazu-yamamoto]

• We can ignore Linux cabal-8.2.2. This happens sometime. I don't know why.
• For Linux cabal-7.10.3, it is strange that tls-1.4.1 is used instead of tls-1.5.0.

[thomasjm]

I notice that the successful test run on master right now also contains tls-1.4.1 in the logs for the Linux cabal-7.10.3 build...

[kazu-yamamoto]

@snoyberg Though CI fails, we don't have to worry about it too much in this case. So, if you like, would you please merge this PR?

[snoyberg]

Just an FYI, this is a larger PR than I anticipated, and I'm a bit swamped right now. I probably won't have a chance to review until next week. Sorry for the delay.

[kazu-yamamoto]

CI passed on master. I'm sure that CI also passed on this PR if re-run.
Probably, git rebase master and git push -f is a good thing.

[thomasjm]

Just rebased on master again.

Here's a few notes on what this PR does in case it helps:

• Implement the desired leading/trailing edge logic in mkDebounce and add a corresponding setting to DebounceSettings.
• For testability, move the core mkDebounce to a .Internal module and make it accept the baton and the delay function as arguments (so that tests can manipulate them).
• Comment out the existing tests (since there's a note saying they're unstable) and re-enable the test suite.
• Add some detailed tests of every mkDebounce configuration.

[snoyberg]

Sorry for the delay in review. Code looks nice and well structured, thank you! And I especially appreciate the tests. I've added some inline comments.

[thomasjm]

Thanks for the review! I resolved everything except for one comment about the details of the semantics.

For that one I updated the docs to be as clear as possible but it would be good if you could take another look. After thinking about it I'm wondering if the distinction between Leading and LeadingAndTrailing is useful or if perhaps LeadingAndTrailing should be removed.

[thomasjm]

(If LeadingAndTrailing were removed then you would be guaranteed that the action will never be invoked more frequently than once every debounceFreq microseconds, which seems nice.)

[kazu-yamamoto]

I've run CI again resulting in green!

[thomasjm]

After consideration I decided to remove LeadingAndTrailing. The distinction between it and Leading is quite small and confusing, and the utility is questionable.

I converted the API to export value-level identifiers instead, so we can re-add it later if we decide we want it.

[snoyberg]

LGTM, just one minor comment

[thomasjm]

Okay, all feedback is addressed now. Thanks @snoyberg !

[snoyberg]

Thanks!