Migrate patterns.js tests#20
Conversation
test/__tests__/patterns.js
Outdated
| // MIGRATION: Error is no longer reported when there are other matches, even with | ||
| // `--error-on-unmatched-pattern`. Adjusted the stderr snapshot for now. | ||
| // Should we change the logic to report the invalid patterns regardless? | ||
| // Changed tha status to 1. |
There was a problem hiding this comment.
Looking at the code in the rewrite we report an error only if nothing is actually matched, not if something is matched but it's subsequently ignored.
There may also be a difference here depending on how many patterns we are trying to match, like does the old CLI throw if any pattern is unmatched, or if every pattern in the list of patterns is unmatched? Worth checking which one it is before deciding what to do here.
Either case worth investigating aligning with the original implementation unless we have a reasonable choice not to, maybe this one is fine, but if we can align it's better to be aligned.
There was a problem hiding this comment.
This test case in the old CLI returns 2, the matches for directory/**/*.js in stdout, and [error] No files matching the pattern were found: "non-existent.js". in stderr. The new version is kinda counter-intuitive, as it returns 1, but doesn't write anything to stderr, because we got some matches from the valid pattern. The code responsible for this is on lines 200-204 in index.ts.
There was a problem hiding this comment.
So we do want to return an error when a single pattern isn't finding any non ignored files within the list of patterns, right? 🤔 Or is whether the found files are ignored or not not a factor in the old CLI?
There was a problem hiding this comment.
Let's consider a case where there are two patterns, one that generates some matches, and one that doesn't. In my opinion, the latter isn't indicative of a failure – there simply aren't any files that match the pattern, and that's fine. So I think the best course of action would be to return 0 (indicating success), write the matches to stdout, and write nothing to stderr.
In this paradigm (not sure whether it's the right word here), even having a single pattern that doesn't match anything wouldn't be a failure – it would simply write nothing to stdout and stderr, and return 0. We should only write to stderr (in both one pattern and two patterns cases) when we set the --error-on-unmatched-pattern flag, also including which pattern(s) didn't match in the error message.
What do you think? It might not be as simple as I wrote down here as from what I see the CLI is very strict when it comes to returning 0, as in there are many edge cases that need to be met for that to happen. Not sure how significant of a refactor would be needed to loosen it up. This is naturally assuming that you agree with my proposal here, which you don't have to. I spent a lot less time in this project than you, so I'm definitely missing some key ideas and details.
There was a problem hiding this comment.
I don't have as much context as I had when I wrote this code, but it looks like erroring on unmatched patters is the default behavior in v3 and v4.
I think what you are proposing makes sense to me, but since this aims to be a largely backwards compatible rewrite (though not fully) I think if we can align with v3 it seems better for our goals with this 🤔
I don't remember what the difference is exactly here, we only error if the sum of all globs didn't find any matches, while v3 errors if any globs didn't find any matches? If so that's a bit problematic to handle as we would probably want some extra metadata for this from tiny-readdir-glob, to avoid searching with each glob individually and then merging their results, but it should be doable.
There was a problem hiding this comment.
Maybe we should add a comment here and investigate v3 vs v4 (and decide whether to add extra metadata to tiny-readdir-glob) in a separate PR so we could land this one? From when I first worked on this, I seem to remember there were some problems with --error-on-unmatched-pattern too, so it would make sense to research within a smaller scope.
|
Probably worth linking to the source files for reference in the future. |
It sounds like there may be a bug in
Yeah seems fine, no patterns no success. |
I'm down to look into that throughout the week, unless you'd like to handle it? I see there are no tests for |
I'll take any help available, but even just making a failing test that reproduces the problem is 80% toward solving it. But also feel free to leave it to me if you think your time is better spent on another area. |
That's what I wanted to start with – a failing test case. I'm gonna go ahead and tentatively put this on my plate. In case something more urgent pops out throughout the week and I won't be able to look into it, I'll let you know either here or in the Discord server. |
fabiospampinato
force-pushed
the
main
branch
4 times, most recently
from
393ff5f to
11afbb2
Compare
|
After updating Notes:
|
|
Thanks! |
2 participants
Please see the
// MIGRATIONcomments on what changes were needed and questions regarding the implementation. Naturally I'm going to remove those when we finalize this PR.The commented test cases are:
!that don't seem supported any more? Maybe there's some flag that I missed?The migrated test: https://github.com/prettier/prettier/blob/main/tests/integration/__tests__/patterns.js.
The migrated snapshots: https://github.com/prettier/prettier/blob/main/tests/integration/__tests__/__snapshots__/patterns.js.snap.
The migrated fixtures: https://github.com/prettier/prettier/tree/main/tests/integration/cli/patterns and https://github.com/prettier/prettier/tree/main/tests/integration/cli/patterns-special-characters.