Document all the things... #291
Conversation
| Stores the provided application instance so that tests being ran will be aware of the application under test. | ||
|
|
||
| Required by `setupApplicationContext` method. | ||
| Used by `setupContext` and `setupRenderingContext` when present. |
There was a problem hiding this comment.
should we use - prefixes here to make that a markdown list?
| let { tagName, type } = el; | ||
| /** | ||
| @private | ||
| @method isFormControl |
There was a problem hiding this comment.
btw with documentation the @method attribute is inferred from the function name, so this is not necessarily needed afaik
There was a problem hiding this comment.
Ok, should I remove it across the board?
There was a problem hiding this comment.
you might want to verify my statement, but unless you see any additional value in having the function name repeated here I see no reason for keeping them
There was a problem hiding this comment.
Yep, just confirmed. Neither documentation nor eslint need the @method.
| /** | ||
| @private | ||
| @method __click__ | ||
| @param {Element} element the element to trigger events on |
There was a problem hiding this comment.
description sounds like it was copy pasted from another function... 😉
There was a problem hiding this comment.
Hmm, it seems correct to me? The element arg is the thing we trigger all the various events (focusin, focus, click, etc) on...
There was a problem hiding this comment.
sure, though "trigger events on" sounds like something that the description for triggerEvent() should say, not necessarily the docs for click()
There was a problem hiding this comment.
Well, a “click” is actually many events 🤔
But either way, I take your point. I should also make that clearer in the click docs too.
| @method settled | ||
| @returns {Promise<void>} resolves when settled | ||
| */ | ||
| export default function settled() { |
There was a problem hiding this comment.
unrelated to this PR but I'm wondering if we should implement this using waitUntil() and moving the backoff algorithm into waitUntil() 🤔
There was a problem hiding this comment.
Yep, I had the same thought actually. Will work on that after these changes have landed.
| setupRenderingTest(hooks); | ||
|
|
||
| test('does something awesome', function(assert) { | ||
| render(hbs`{{awesome-sauce}}`); |
There was a problem hiding this comment.
this example is not using async/await, which seems a little strange as it's the default now 🤔
There was a problem hiding this comment.
Totally agreed. Will update.
| @@ -38,6 +95,12 @@ export function pauseTest() { | |||
| return context.pauseTest(); | |||
| } | |||
|
|
|||
| /** | |||
| Resumes a test previously paused by `return pauseTest()`. | |||
There was a problem hiding this comment.
I prefer await pauseTest() since return pauseTest() will make resumeTest() essentially impossible
| let result = validateErrorHandler(); | ||
| assert.ok(result.isValid, result.message); | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
I would prefer it if the @example block would be moved below the function signature documentation
There was a problem hiding this comment.
Totally agreed. Will update.
Includes a small tweak to the implementation (no longer receives an object)...
Kept as "private" for now, but we intend to make it public in a future PR...
Also make `@method` show up as a linting error (though you _could_ use `@do-not-use-redundant-method-tag methodNameHere` but that seems _obviously wrong and easy to spot in PR review).
Also makes it clear that the specific list of fired events may change over time as needed.
Also use alternate JSDoc style with prefixed `*` on each line. Without this the indentation of the example block is lost.
Adds API documentation for all public APIs (and some private ones).
Also enables the
require-jsdoceslint rule to ensure all functions are documented going forward...Closes #285