Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive after committing a suggestion #66

Open
informeti opened this issue Sep 2, 2024 · 4 comments
Open

False positive after committing a suggestion #66

informeti opened this issue Sep 2, 2024 · 4 comments

Comments

@informeti
Copy link

informeti commented Sep 2, 2024

I just tried out this action, and formatting and reporting both seem to work out-of-the-box. Excellent!

But when I committed the suggestion in GitHub, the changed lines were reported again as an error, except that the diff doesn't really show a difference.

It seems to be due to a missing line at the end of the file, which Biome seems to unconditionally require... If my understanding is correct, this action, reviewdog or biome strips the empty line at the end of the file.

Truth be told, I'd rather have Biome accept this, but afaik Biome is a bit conservative on that side of things.


Before committing the suggestion (so far, so good):
image

After committing the suggestion (not good):
image

Workflow file:

name: check

on:
  pull_request:

jobs:
  biome:
    runs-on: ubuntu-latest
    permissions:
      contents: read
      pull-requests: write
    steps:
      - uses: actions/checkout@v4
      - uses: mongolyy/reviewdog-action-biome@v1
        with:
          github_token: ${{ secrets.github_token }}
          reporter: github-pr-review
          fail_on_error: true

Log:

Run mongolyy/reviewdog-action-biome@v1
Run biomejs/[email protected]
Run reviewdog/[email protected]
Run set -eu
🐶 Installing reviewdog ... https://github.com/reviewdog/reviewdog
Run set -eu
📖 reviewdog -h
Run $GITHUB_ACTION_PATH/script.sh
 Running Biome with reviewdog 🐶 ...
Run reviewdog/action-suggester@v1
Run set -euo pipefail
🐶 Installing reviewdog ... https://github.com/reviewdog/reviewdog
Run set -euo pipefail
📖 reviewdog -h
Run set -euo pipefail
  set -euo pipefail
  . "$GITHUB_ACTION_PATH/script.sh"
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    INPUT_GITHUB_TOKEN: ***
    INPUT_TOOL_NAME: Biome
    INPUT_LEVEL: error
    INPUT_FILTER_MODE: added
    INPUT_FAIL_ON_ERROR: true
    INPUT_REVIEWDOG_FLAGS: 
    INPUT_CLEANUP: true
Saved working directory and index state WIP on (no branch): 57a96d7 Merge 9fc052bdd7b9084[33](snip#step:3:39)30de0bbc456e6ccb9436770 into 560b9777d43c5fa6cc80342c6fd4fa50e878fe28
ops/main.ts:3:-export * as organization from "./organization/main";
ops/main.ts:3:+export * as organization from "./organization/main";
reviewdog: input data has violations
Error: Process completed with exit code 1.

PS: Thank you for this wonderful action!

@mongolyy
Copy link
Owner

mongolyy commented Sep 3, 2024

@informeti
Thank you for using action! 🤩
I'm happy that you were pleased.

After committing the suggestion, please try checking it out.
Could you try running biome check or biome ci commands to see if there are any differences?

On the back end, if there are any differences when action run biome check --write, a commit suggestion will be made.
I'm wondering if there are any differences in things like line endings.

@informeti
Copy link
Author

informeti commented Sep 4, 2024

Hi @mongolyy, thank you for your quick response!

So far, I haven't noticed any inconsistency between biome check and biome ci.

After committing the suggestion and pulling the difference locally, the result is the following:

$ biome check main.ts
main.ts format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Formatter would have printed the following content:

    1 1 │   export * as deployment from "./deployment/main";
    2 2 │   export * as globalServices from "./global-services/main";
    3   │ - export·*·as·organization·from·"./organization/main";
      3 │ + export·*·as·organization·from·"./organization/main";
      4 │ +


Checked 1 file in 809µs. No fixes applied.
Found 1 error.
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.


$ biome ci main.ts
main.ts format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ File content differs from formatting output

    1 1 │   export * as deployment from "./deployment/main";
    2 2 │   export * as globalServices from "./global-services/main";
    3   │ - export·*·as·organization·from·"./organization/main";
      3 │ + export·*·as·organization·from·"./organization/main";
      4 │ +


Checked 1 file in 1363µs. No fixes applied.
Found 1 error.
ci ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.

I also tried to "reproduce" what the action would be doing. I resetted all changes and introduced the formatting error again. As expected, both biome check and biome ci recognize the error correctly:

 biome check main.ts
main.ts format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Formatter would have printed the following content:

    1 1 │   export * as deployment from "./deployment/main";
    2   │ - ·export·*·as·globalServices·from·"./global-services/main";
    3   │ - export·*·as·organization·from·"./organization/main";
      2 │ + export·*·as·globalServices·from·"./global-services/main";
      3 │ + export·*·as·organization·from·"./organization/main";
      4 │ +


Checked 1 file in 952µs. No fixes applied.
Found 1 error.
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.


$ biome ci main.ts
main.ts format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ File content differs from formatting output

    1 1 │   export * as deployment from "./deployment/main";
    2   │ - ·export·*·as·globalServices·from·"./global-services/main";
    3   │ - export·*·as·organization·from·"./organization/main";
      2 │ + export·*·as·globalServices·from·"./global-services/main";
      3 │ + export·*·as·organization·from·"./organization/main";
      4 │ +


Checked 1 file in 819µs. No fixes applied.
Found 1 error.
ci ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks

Then, I did what I expect the action would do:

$ biome check --write main.ts
Checked 1 file in 1001µs. Fixed 1 file.
$ biome check main.ts
Checked 1 file in 1204µs. No fixes applied.
$ biome ci main.ts
Checked 1 file in 863µs. No fixes applied.
$ xxd main.ts
00000000: 6578 706f 7274 202a 2061 7320 6465 706c  export * as depl
00000010: 6f79 6d65 6e74 2066 726f 6d20 222e 2f64  oyment from "./d
00000020: 6570 6c6f 796d 656e 742f 6d61 696e 223b  eployment/main";
00000030: 0a65 7870 6f72 7420 2a20 6173 2067 6c6f  .export * as glo
00000040: 6261 6c53 6572 7669 6365 7320 6672 6f6d  balServices from
00000050: 2022 2e2f 676c 6f62 616c 2d73 6572 7669   "./global-servi
00000060: 6365 732f 6d61 696e 223b 0a65 7870 6f72  ces/main";.expor
00000070: 7420 2a20 6173 206f 7267 616e 697a 6174  t * as organizat
00000080: 696f 6e20 6672 6f6d 2022 2e2f 6f72 6761  ion from "./orga
00000090: 6e69 7a61 7469 6f6e 2f6d 6169 6e22 3b0a  nization/main";.

And, as expected, the newline is there and biome doesn't complain.

So I'm a bit confused... what could be removing the empty line?

EDIT: Maybe irrelevant, but I have an .editorconfig in the repository root, which indicates insert_final_newline = false. AFAIK Biome takes over some editorconfig properties, but ignores this one though.

@mongolyy
Copy link
Owner

mongolyy commented Sep 4, 2024

@informeti
Thank you more information!

The problem has been reproduced at #67 .

I think the settings for the tool called reviewdog used in this action are a little suspicious.

I think reviewdog/reviewdog#1580 is similar.
However, setting filter_mode: diff_context did not solve this problem at #67.

I will investigate this in more detail this weekend.

@mongolyy
Copy link
Owner

mongolyy commented Sep 8, 2024

@informeti
I did some more research.

I came to the conclusion that there might be a problem with the tool called reviewdog that this tool uses.

Therefore, I created an issue and wait for the response.
reviewdog/reviewdog#1927

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants