fix: allow empty result from changed vcs files

[antogyn]

Summary

Allow an empty result when checking for changed files, which can later be allowed or not depending on the --no-errors-on-unmatched flag

Fixes #1774

Test Plan

To be done

[netlify]

✅ Deploy Preview for biomejs ready!

Name	Link
🔨 Latest commit	5e3fe48
🔍 Latest deploy log	https://app.netlify.com/sites/biomejs/deploys/65e5f357982d7000082cf803
😎 Deploy Preview	https://deploy-preview-1776--biomejs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (🟢 up 1 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 93 (no change from production) PWA: - View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[ematipico]

This looks good to me, I am happy to merge it whenever you want

[antogyn]

It doesn't actually fix the issue, and returns an other error:

flags/invalid ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Missing argument <INPUT>

It seems to be returned at the start of traverse, I'm looking into it

[antogyn]

In my opinion this check should instead be done at the parsing level if possible, for example in crates/biome_cli/src/commands/mod.rs:

        /// Single file, single path or list of paths
        #[bpaf(positional("PATH"), many)]
        paths: Vec<OsString>,

paths should ideally be validated right here, eg something like this, using some from bpaf:

        /// Single file, single path or list of paths
        #[bpaf(positional("PATH"), some("No path was specified"))]
        paths: Vec<OsString>,

unless there are cases where it's ok to have no path? Wdyt?

[antogyn]

I've updated the PR with a new proposal

It slightly changes the output if there is no provided path, as it's directly validated by bpaf:

Error: No path was provided

instead of

flags/invalid ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Missing argument <INPUT>

and it does fix the issue this time 🎉

[ematipico]

unless there are cases where it's ok to have no path?

There's a case, which is when users need/want to process files via standard in.

[antogyn]

Well, it breaks tests that make use of --stdin-file-path, and it looks like a legit case of not providing paths, so my proposal doesn't work

I guess this check:

if inputs.is_empty() && execution.as_stdin_file().is_none() {
        return Err(CliDiagnostic::missing_argument(
            "<INPUT>",
            format!("{}", execution.traversal_mode),
        ));
    }

must stay somewhere

[antogyn]

I've updated the PR with the most straightforward solution, which allows empty paths if --no-errors-on-unmatched is set, by just updating the condition

inputs.is_empty() && execution.as_stdin_file().is_none()

to

inputs.is_empty() && execution.as_stdin_file().is_none() && !cli_options.no_errors_on_unmatched

It fixes the issue, but I'm not sure it's acceptable

[ematipico]

I believe this check should stay, but we need to add the check if no_errors_on_unmatched is true

[antogyn]

I've re-added this check, but now it's done twice - once here, and once in traverse.rs

[antogyn]

Is there anything else preventing it from being merged?

[ematipico]

Is there anything else preventing it from being merged?

A test plan :)

[ItamarShDev]

I have it too and it prevents me from integrating Biome to the company sadly. will wait

[ray-peters]

In the same boat with @ItamarShDev
Would love to see this bug fix sooner than later