test(utils): Add readFixture and mockTemplate helper functions
#1093
Conversation
ExE-Boss
changed the title
readFixture and mockTemplate helper functionsreadFixture and mockTemplate helper functions
ExE-Boss
force-pushed
the
test/utils/read-fixture
branch
from
233c349
to
f59cc28
Compare
ExE-Boss
force-pushed
the
test/utils/read-fixture
branch
from
f59cc28
to
6d25c72
Compare
There was a problem hiding this comment.
@ExE-Boss: thanks for pushing this. I continue to think that these are really useful test utilities. I think I had a couple of nits, but overall I think they're well written and it is great that you've included tests.
My request for changes has to do with all the type comments you've added. There are a couple of errors in the comments that will need to be fixed at a minimum. But there's also the larger issue of whether we want to be adding this kind of type information to this code. I don't have much of an opinion myself, but I'm not sure it makes sense to go to the effort of adding all this information unless there is consensus among the active contributors (and I don't include myself in that list) that the type information adds enough value to be worth it.
That is maybe a question for @wbamberg and @escattone?
My view: I don't obviously see a need for these, no. But more strongly, I'd very much prefer to see a single PR for a single purpose. This PR is to add the helper functions, so I'd prefer if it did just that. Then we could make the type comments a separate discussion. I think this is good as a general policy: it's easier for reviewers, and it's easier to have clear conversations and decisions about what we want to have in the repo, and to prioritise this work too. |
|
I totally agree with @wbamberg and @davidflanagan, but wanted to add one more thing. I'm not a fan of the type comments, and would personally prefer that they were not added. In my opinion, they're a burden and add little, if any, insight to that offered by the prose comments (which I do like). |
|
While working on this, I discovered an unrelated issue which I’m fixing in #1057 (review). |
There was a problem hiding this comment.
I like this version much better than the last. I'm requesting changes to the default encoding of readFixture(), and I think that there might still be a few problems with types for the assertion functions.
Also: I don't think you've actually explained the point of the comments. I've got enough experience with Flow to find these comments readable and kind of helpful, though I suspect that not all developers would agree with that. I assume that you're using these yourself with vscode as described in https://medium.com/@trukrs/type-safe-javascript-with-jsdoc-7a2a63209b76 Is that right?
I'm really not sure how I feel about that. I really like Flow and I understand the benefits of typed JavaScript. But I don't know what is going to happen when some contributors to a project are using type checking and some are not. Basically, I worry that changes to this code will break the types, and then you'll have vscode reporting errors to you that no one else sees or cares about. And if you then submit PRs to fix them, we'll be taking time to review those PRs. Basically: I like typed JS, but I fear that in this case it will slow us down, and that for this relatively small codebase it is not really worth adding them.
On the other hand, if the type information is just there for human readers and not for automated tools, then it seems fine to add this extra detail in the comments.
Finally, I'd really like to see a commit message that is at least a paragraph long explaining the changes in the PR. This is so that we get useful information from git log once we merge.
| }); | ||
|
|
||
| it('`readFixture` works', function() { | ||
| let result = String(readFixture('utils-test/fixture.txt')).trim(); |
There was a problem hiding this comment.
Does this work on Windows, since you've been looking into that?
There was a problem hiding this comment.
Yes.
I use Windows, so I know that my code works on Windows.
ExE-Boss
force-pushed
the
test/utils/read-fixture
branch
from
e457eff
to
de59e84
Compare
| * Asserts deep equality of actual and expected. | ||
| * | ||
| * @template T Type of the resolved value. | ||
| * @param {T|Promise<T>} x A promise which resolves to the actual value. |
There was a problem hiding this comment.
The type parameter has to be specified here, otherwise the type would collapse into any, instead of T|Promise<T>.
| * | ||
| * The `.json` file extension can be omitted. | ||
| * | ||
| * @returns {T} The parsed JSON payload. |
There was a problem hiding this comment.
This is added so that the type cast gets performed automatically.
| * @param {string|null} [encoding] | ||
| * The file encoding. Set to `null` to load the file as a binary `Buffer`. | ||
| * | ||
| * @returns {string} |
There was a problem hiding this comment.
Unfortunately, JSDoc doesn’t provide any way to define function overloads, so the return type will always show up as a string, unless we create utils.d.ts or migrate this file to TypeScript.
|
/ping @davidflanagan |
Extracted from #1073 and #1077, neither of which seem to be getting anywhere.
review?(@a2sheppy, @chrisdavidmills, @davidflanagan, @Elchi3, @wbamberg)