-
Notifications
You must be signed in to change notification settings - Fork 22
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
Rename horizons to auto-sample-conditions #225
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't manually check anything, but the documentation and naming changes look fantastic. They're definitely much clearer. Great test updates as well!
Only tiny optional nits.
🎉
| `0.5ms` | Is A faster or slower than B by at least 0.5 milliseconds? | | ||
|
||
In the following example, we have set `--horizon=10%`, meaning we are interested | ||
### Auto sample conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe both? "Auto sample stopping conditions" - unless conditions can do more than just stop? Also no action required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to be 100% consistent with the phrase "auto sample conditions".
README.md
Outdated
conditions_. This is controlled by the `--auto-sample-conditions` flag or the | ||
`autoSampleConditions` JSON config option. | ||
|
||
An auto sample condition condition can be thought of as a point of interest on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: condition
intended twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! Thanks, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
condition
intended twice?
"items": { | ||
"type": "string" | ||
}, | ||
"type": "array" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Very helpful!
@@ -768,6 +768,22 @@ suite('config', () => { | |||
'config.benchmarks[0].packageVersions requires property "label"' | |||
); | |||
}); | |||
|
|||
test('Error to use both horizons and autoSampleConditions', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
9e8bf54
to
cc3bb9f
Compare
cc3bb9f
to
20a2b34
Compare
|
I never liked the name "horizons" (except on the day I chose it I guess). I think a much clearer name is "auto sample conditions".
This changes the
--horizon
flag and thehorizons
JSON config setting (yes... one was singular, the other plural) to--auto-sample-conditions
andautoSampleConditions
respectively.It's backwards compatible, with a warning printed if you use the old style.
Related to #221