feat(valid-expect): supporting automatically fixing adding async in some cases #1579
Conversation
There was a problem hiding this comment.
looks like a good start, but this isn't quite the right approach to being incremental 😅 - a fix should address the full issue.
What I meant by incremental is landing a fixer for e.g. expect.extend or functions without any nesting or a single expect, then digging into advanced cases like "what if you have a function within your test function?"; but also you don't have to do it like that either - if you want to tackle more in a single PR that's great! based on what you've done here for example, I think you might just need to switch to returning an array of fixes and then this might be landable
|
I misunderstood the concept of incremental improvement. |
|
I'm encountering a situation where the logic to add 'await' to all expects isn't functioning correctly when there are multiple asynchronous expectations. |
|
@y-hsgw push up your code and I can take a look. That also sounds like a good example of where we might want to be interactive: if you're having trouble with the fixing, consider focusing on having it just bail out instead for now. |
|
I might have misunderstood. It's likely that the following sequence of fixes will occur: |
|
@y-hsgw I think you might be misunderstanding - that relates to when there are conflicts:
The fixer works by taking (via the return) an array of fix operations to apply which make up a single fix - if there is only one operation, then you can just return that but you can think about it as being an array of operations at all times. So when those docs talk about "applying all fixes" and "applies only the first fix" they're referring to the high-level fix aka "a collection of fix operations", which is what you're building here - the issue is right now you're returning two seperate fix operations depending on conditions, when you want to be returning a single array with both operations added conditionally depending on if they're needed for the particular code under lint. |
|
@G-Rath 2024-05-10.22.04.36.mov |
|
Yes because as the tests show via the It should be very trivial to return a single array too :) |
This reverts commit e652a25.
|
@y-hsgw my guess is that it's because you're adding the I would recommend focusing on a single test (you can add |
| const functionExpression = findFirstFunctionExpression(finalNode); | ||
|
|
||
| if (functionExpression && !functionExpression.async) { | ||
| context.report({ | ||
| loc: functionExpression.loc, | ||
| data: { orReturned }, | ||
| messageId: | ||
| finalNode === targetNode | ||
| ? 'asyncMustBeAwaited' | ||
| : 'promisesWithAsyncAssertionsMustBeAwaited', | ||
| node, | ||
| fix(fixer) { | ||
| const targetFunction = | ||
| getNormalizeFunctionExpression(functionExpression); | ||
|
|
||
| return fixer.insertTextBefore(targetFunction, 'async '); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
@G-Rath
I've attempted to modify the logic, but is my understanding correct?
There was a problem hiding this comment.
No this isn't correct, since we're now reporting an error for the function which is 1. not the actual issue and 2. reported per expect (which itself already reports an error).
There was a problem hiding this comment.
I see. Do I need to add both async and await within a single context.report?
There was a problem hiding this comment.
Thanks.
if I add the async to the function, then both awaits get added correctly.
Is my understanding of adding async in the following code correct?
fixer.insertTextBefore(targetFunction, 'async ');There was a problem hiding this comment.
Yup that looks like it's probably right, though it's not the only way.
I have addressed #1140 (comment) 1 and 2.
Relates to #1140