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

🐛 Github Reporter not generating annotations correctly #3148

Closed
1 task done
tommoyang opened this issue Jun 10, 2024 · 5 comments · Fixed by #3281
Closed
1 task done

🐛 Github Reporter not generating annotations correctly #3148

tommoyang opened this issue Jun 10, 2024 · 5 comments · Fixed by #3281
Assignees
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug workaround

Comments

@tommoyang
Copy link

tommoyang commented Jun 10, 2024

Environment information

Commands are being run in a Warpbuild Github Actions runner (specifically warp-ubuntu-latest-x64-2x), but i've tested and this happens on ubuntu-latest too

CLI:
  Version:                      1.8.0
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "unknown"
  JS_RUNTIME_VERSION:           "v18.19.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "yarn/4.1.0"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 false

Workspace:
  Open Documents:               0

What happened?

  1. Set up biome in a project with linting errors
  2. Run yarn biome ci --reporter=github in an actions workflow/step

Expected result

Expected: Annotation output should turn into an annotation in the Github Actions execution logs/Files Changed view

Actual: The annotation is output as a string and nothing happens. i.e. I see this in the console:

::error title=lint/correctness/useJsxKeyInIterable,file=path/to/file.tsx,line=36,endLine=36,col=20,endColumn=48::Missing key property for this element in iterable.
Error: Process completed with exit code 1.

Instead of this, and all the other annotations stuff:

Error: Missing key property for this element in iterable.

I can confirm that the annotation format is correct - if I manually echo the output it turns into an annotation correctly, and it's only from yarn biome ci that it fails to work.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@tommoyang tommoyang changed the title 🐛 Github Reporter not generating annotations 🐛 Github Reporter not generating annotations correctly Jun 10, 2024
@ematipico
Copy link
Member

Yeah I have been trying to understand why it hasn't been working properly, and I can't figure out why

@Sec-ant Sec-ant added S-Help-wanted Status: you're familiar with the code base and want to help the project A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug labels Jun 10, 2024
@Sec-ant
Copy link
Member

Sec-ant commented Jun 20, 2024

I think colors is the reason why the annotation doesn't work properly. Here is a working one I tested in my fork (not a fork, the website repo) with biome ci --reporter=github --colors=off using the v1.8.2 biome release, it correctly generated all the annotations.

@ematipico ematipico removed the S-Bug-confirmed Status: report has been confirmed as a valid bug label Jun 20, 2024
@tommoyang
Copy link
Author

Tested, it's also working for me! Thanks for the fix.

I think if Github annotations are going to be generated it might be better to force colours off as opposed to setting a flag to explicitly turn them off, but i'm happy to leave that as just a note and close this issue.

@Sec-ant
Copy link
Member

Sec-ant commented Jun 24, 2024

I think if Github annotations are going to be generated it might be better to force colours off as opposed to setting a flag to explicitly turn them off

I completely agree with you.

One thing is turning colours off also means that the colours for summary details/total are also off:

impl fmt::Display for SummaryDetail {
fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> {
if self.0 > 0 {
fmt.write_markup(markup! {
" Fixed "{Files(self.0)}"."
})
} else {
fmt.write_markup(markup! {
" No fixes applied."
})
}
}
}
struct SummaryTotal<'a>(&'a TraversalMode, usize, &'a Duration);
impl<'a> fmt::Display for SummaryTotal<'a> {
fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> {
let files = Files(self.1);
match self.0 {
TraversalMode::Check { .. } | TraversalMode::Lint { .. } | TraversalMode::CI { .. } => {
fmt.write_markup(markup! {
"Checked "{files}" in "{self.2}"."
})
}
TraversalMode::Format { write, .. } => {
if *write {
fmt.write_markup(markup! {
"Formatted "{files}" in "{self.2}"."
})
} else {
fmt.write_markup(markup! {
"Checked "{files}" in "{self.2}"."
})
}
}
TraversalMode::Migrate { write, .. } => {
if *write {
fmt.write_markup(markup! {
"Migrated your configuration file in "{self.2}"."
})
} else {
fmt.write_markup(markup! {
"Checked your configuration file in "{self.2}"."
})
}
}
TraversalMode::Search { .. } => fmt.write_markup(markup! {
"Searched "{files}" in "{self.2}"."
}),
}
}
}

This is not a big deal, but the ideal fix for this should be only switching off colours for the "annotation" part.

I'd like to reopen this as a reminder. Either forcing colours off entirely in this mode, or only keeping annotataions colorless is an acceptable fix.

@Sec-ant Sec-ant reopened this Jun 24, 2024
@ematipico
Copy link
Member

I'd like to reopen this as a reminder. Either forcing colours off entirely in this mode, or only keeping annotataions colorless is an acceptable fix.

I think removing colours based on the reporter is the way to go. Keeping annotation colourless feels too complex, and I don't envision future cases where we need such a sophisticated feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug workaround
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants