Skip to content

Conversation

chengxilo
Copy link
Contributor

@chengxilo chengxilo commented Jul 23, 2025

🤔 What's changed?

  1. replace the previous static check in workflow to golangci-lint
  2. code was edited to meet the requirement of golangci-lint

⚡️ What's your motivation?

Fix #696
and my presonal preference.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:


This text was originally generated from a template, then edited by hand. You can modify the template here.

Copy link

codecov bot commented Jul 23, 2025

Codecov Report

❌ Patch coverage is 24.86486% with 278 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.41%. Comparing base (153db4e) to head (24db0c1).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
internal/formatters/fmt_pretty.go 20.49% 93 Missing and 35 partials ⚠️
internal/formatters/fmt_progress.go 12.90% 19 Missing and 8 partials ⚠️
run.go 22.85% 20 Missing and 7 partials ⚠️
stacktrace.go 21.42% 17 Missing and 5 partials ⚠️
cmd/godog/internal/cmd_root.go 0.00% 21 Missing ⚠️
flags.go 0.00% 9 Missing and 5 partials ⚠️
internal/builder/builder.go 46.66% 7 Missing and 1 partial ⚠️
fmt.go 40.00% 4 Missing and 2 partials ⚠️
internal/parser/parser.go 40.00% 4 Missing and 2 partials ⚠️
cmd/godog/internal/cmd_run.go 0.00% 5 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
- Coverage   83.21%   73.41%   -9.80%     
==========================================
  Files          28       41      +13     
  Lines        3413     4292     +879     
==========================================
+ Hits         2840     3151     +311     
- Misses        458      953     +495     
- Partials      115      188      +73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chengxilo chengxilo marked this pull request as ready for review July 23, 2025 13:47
@chengxilo
Copy link
Contributor Author

I’ve noticed that the Codecov workflow fails on this pr while its changes don’t significantly affect code coverage. So I think for this pr maybe it is a good idea to igonore the Codecov report.

@chengxilo chengxilo changed the title Use golangci lint ci: use golangci lint Jul 23, 2025
@Saurabh2402
Copy link

Hi @chengxilo & @l3pp4rd

Thanks for taking initiative on my issue #696, the linter integration! I have a few questions about the implementation:

  • Configuration clarity: Since no .golangci.yml is provided, which default linters/rules is the GitHub Action using? This might lead to inconsistent local vs CI behavior.

  • Code quality concerns:

    • I noticed some patterns that might be making the code unnecessarily complex:
      • Using _, _ = fmt.Fprint() everywhere - this ignores potentially important errors
      • Adding defer file.Close() error checking in places where it might not be needed
    • These changes seem to be driven by linter warnings rather than actual code improvement needs.
  • Offer to help:

    • Since I originally raised this issue and have experience with golangci-lint configuration, I'd be happy to:
    • Create a balanced .golangci.yml configuration
    • Refine the current implementation
    • Collaborate on making the linting more practical

@l3pp4rd What do you think? Would a new PR with proper configuration be helpful?

@chengxilo
Copy link
Contributor Author

Hi @Saurabh2402,

Thanks for your feedback!

Regarding the linters: here are the default ones used by golangci-lint – https://golangci-lint.run/usage/linters/#enabled-by-default. While I don't think this usually causes inconsistencies between local and CI environments, I agree that adding a configuration file would be a good idea if we want more control over which linters are enabled or disabled. Probably you could tell me which lint you want to enable, so I can add the configuration file with those lints enabled.

On the code quality concerns: I tend to use _, _ = fmt.Fprintf() when printing error messages (most of them are for error output), since there's often not much we can do if an error occurs at that point. That said, you're right—there are some error values I probably shouldn't ignore. I'll go through and fix those (for the non-error part).

Regarding handling defer file.Close(), you might be right—I’ll remove the redundant error checks if they’re not useful.

As for future PRs: if the code isn’t too terrible, maybe the best way would be to leave a review or you can feel free to create a new pr.

Thanks again!

@chengxilo chengxilo marked this pull request as draft July 25, 2025 20:02
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

Successfully merging this pull request may close these issues.

🔧 Integrate golangci-lint for Static Code Analysis
2 participants