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

feat: add triangle latency summand #18

Merged
merged 4 commits into from
Aug 10, 2022

Conversation

roertbb
Copy link
Contributor

@roertbb roertbb commented Aug 1, 2022

What was done in this PR:

  • Added triangle summands.

Additionally:

Resolves #19

@roertbb roertbb changed the title add triangle and square latency subcommands Add triangle and square latency subcommands Aug 2, 2022
@roertbb roertbb force-pushed the additional-latency-subcommands branch from f176ccd to ea9d51b Compare August 2, 2022 19:38
@roertbb roertbb changed the title Add triangle and square latency subcommands feat: add triangle and square latency subcommands Aug 2, 2022
@roertbb roertbb marked this pull request as ready for review August 2, 2022 19:41
@kffl
Copy link
Owner

kffl commented Aug 2, 2022

This PR adds triangle and square latency summands. After submitting the PR I've noticed that someone already submitted a PR for square latency summand in #17. I'll wait until it's merged and adjust my changes to add triangle latency summand only. Will it work for us, wdyt?

Thank you for implementing both of them! #17 just got merged

Good catch on that sawAmplitute typo. Since it is an exported field in an exported struct LatencyCfg and I'm rather strict about adhering to SemVer, it seems that we are going to arrive at v1 rather quickly. Putting this fix aside, I have already planned altering the proxy Start() and Stop() methods to make them more intuitive, so the timing for a major version bump is pretty great.

@roertbb roertbb force-pushed the additional-latency-subcommands branch from ea9d51b to 66a00b8 Compare August 3, 2022 05:00
@kffl kffl changed the title feat: add triangle and square latency subcommands feat: add triangle latency summand Aug 3, 2022
Copy link
Owner

@kffl kffl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking the time to prepare this PR. I've included my remarks regarding potential simplification of the triangle wave function. Let me know if you would like to refactor it. If not - no worries, this is by no means obligatory.

lib/sawtooth_test.go Show resolved Hide resolved
lib/triangle.go Outdated Show resolved Hide resolved
@roertbb roertbb force-pushed the additional-latency-subcommands branch from 66a00b8 to 15028ae Compare August 5, 2022 16:16
BREAKING CHANGE:
Regarding only the improvement in LatencyCfg
@roertbb roertbb force-pushed the additional-latency-subcommands branch from 15028ae to 007a992 Compare August 5, 2022 16:17
@roertbb roertbb requested a review from kffl August 5, 2022 16:23
Copy link
Owner

@kffl kffl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for introducing the changes.

args.go Outdated Show resolved Hide resolved
@kffl kffl added this to the v1 Release milestone Aug 7, 2022
@kffl kffl mentioned this pull request Aug 7, 2022
@kffl kffl merged commit 9a4fb6d into kffl:master Aug 10, 2022
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.

Triangle latency summand
2 participants