configurable timeout and delay values #61
Conversation
There was a problem hiding this comment.
This approach, while compact, offers only minimal documentation of a bespoke sub-syntax. I think we ought to treat each value like a first-class option. Doing so will significantly expand the size of the patch, but I'm trying to keep the spirit of the change in mind.
The formatting of yargs "describe" was a true pain also, you can't really setup newlines/it breaks text willy nilly in the middle of words, etc. Each as it's own option would actually make for a better |
|
src/shared/times-option.js
Outdated
| /** | ||
| * Convert from 'afterNav' to 'time-after-nav'. | ||
| * @param {keyof AriaATCIShared.timesOption} optionName | ||
| * @returns string | ||
| */ | ||
| function makeSnakeCasedOption(optionName) { | ||
| const snakeCased = optionName.replace(/[A-Z]/g, cap => '-' + cap.toLowerCase()); | ||
| const optionText = `time-${snakeCased}`; | ||
| return optionText; | ||
| } |
There was a problem hiding this comment.
I kinda have a thing for grep-able source code. I'd much rather maintain a bit of redundancy (e.g. an argument list like "afterNav", "time-after-nav") than an abstraction like this.
There was a problem hiding this comment.
pushed this change in
src/shared/times-option.js
Outdated
| /** | ||
| * @type AriaATCIShared.timesOption | ||
| */ | ||
| export const timesOption = { | ||
| afterNav: 1000, | ||
| afterKeys: 5000, | ||
| testSetup: 1000, | ||
| modeSwitch: 750, | ||
| docReady: 2000, | ||
| }; |
There was a problem hiding this comment.
Stateful modules are a source of tech debt in my experience. This clearly works for our current needs, but at some point in the future (e.g. when we go to write a new kind of test or re-use something), the timesOption singleton may make the change more involved than if we had explicitly passed the value around to begin with. Think we can treat these values a little more like the rest of the command-line arguments?
There was a problem hiding this comment.
It's possible, the reason I was sort of avoiding it is this would necessitate passing these options into modules everywhere, I feel like the usability/dev experience of adding a timeout to it in the future might be pretty large. I'll do this as a separate commit to kind of weigh it out (not having done it yet, it might not be "that bad")
There was a problem hiding this comment.
In the end this didn't feel too terrible. ea8d351
src/shared/times-option.js
Outdated
| /** | ||
| * Convert the times dictionary to an array of strings to pass back to args. | ||
| * @param {AriaATCIShared.timesOption} opts | ||
| * @returns [string] |
There was a problem hiding this comment.
[string] means "an array with exactly one string element"; what we want here is
| * @returns [string] | |
| * @returns {string[]} |
(The compiler isn't flagging this right now, presumably due to the absence of curly braces. I guess it considers that "not JSDoc" rather than "invalid JSDoc".)
There was a problem hiding this comment.
oops! thanks! (still not super used to the jsdoc format)
Fixes #49
Posting this up to check on the approach / acceptability from @jugglinmike