Visitar URL original
Leading vs. trailing edges for Control.Debounce by thomasjm · Pull Request #756 · yesodweb/wai · GitHub
Skip to content

Leading vs. trailing edges for Control.Debounce - #756

Merged
snoyberg merged 11 commits into
yesodweb:masterfrom
codedownio:master
Jul 9, 2019
Merged

snoyberg merged 11 commits into
yesodweb:masterfrom
codedownio:master

Conversation

@thomasjm

@thomasjm thomasjm commented Jun 24, 2019 •

Copy link
Copy Markdown
Contributor

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

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 thomasjm changed the title First pass leading vs. trailing edges for Control.Debounce Leading vs. trailing edges for Control.Debounce Jun 24, 2019
@thomasjm

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

@snoyberg

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

The remaining failures seem to be related to warp

@kazu-yamamoto

Copy link
Copy Markdown
Contributor

Very strange. tls 1.4.1 surely exports CompressionID.

@kazu-yamamoto

Copy link
Copy Markdown
Contributor

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

@thomasjm

Copy link
Copy Markdown
Contributor Author

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

@kazu-yamamoto

Copy link
Copy Markdown
Contributor
  • 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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

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

thomasjm commented Jul 2, 2019

Copy link
Copy Markdown
Contributor Author

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 snoyberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread auto-update/Control/Debounce.hs Resolved
@thomasjm

thomasjm commented Jul 4, 2019

Copy link
Copy Markdown
Contributor Author

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

thomasjm commented Jul 4, 2019

Copy link
Copy Markdown
Contributor Author

(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

Copy link
Copy Markdown
Contributor

I've run CI again resulting in green!

@thomasjm

thomasjm commented Jul 5, 2019 •

Copy link
Copy Markdown
Contributor Author

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 snoyberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one minor comment

@thomasjm

thomasjm commented Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

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

@snoyberg
snoyberg merged commit ae64943 into yesodweb:master Jul 9, 2019
@snoyberg

snoyberg commented Jul 9, 2019

Copy link
Copy Markdown
Member

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants