Skip to content

chore: enable begin rule from thelper #1816

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

Merged
merged 1 commit into from
Mar 17, 2025
Merged

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Mar 9, 2025

Description

Enables begin rule from thelper, it also turns SkipIfNotImplementedErr into a common function

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR enables the begin rule from thelper by reverting the previous disabled configuration in the linter and refactors test files to use a common helper function for handling not-implemented errors.

  • Introduced a new common function (SkipIfNotImplementedErr) in the common package.
  • Replaced duplicate skip helper functions across various test files with the common implementation.
  • Updated the .golangci.yml configuration to remove the disabled begin rules for thelper.

Reviewed Changes

File Description
internal/common/common_testing.go Adds the new common helper function for skipping not-implemented tests.
internal/common/common.go Adds ErrNotImplementedError and removes the old duplicate definition.
.golangci.yml Removes the disabled thelper settings to enable the begin rule.
Various test files in cpu, mem, net, host, process, etc. Replace local skipIfNotImplementedErr with common.SkipIfNotImplementedErr.

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented Mar 9, 2025

That’s my first review by @copilot , interesting to see how accurate it can be .

Copy link
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Actually, skipIfNotImplementedErr has been a long-time concern of mine.

@shirou shirou merged commit d875090 into shirou:master Mar 17, 2025
51 checks passed
@mmorel-35 mmorel-35 deleted the thelper/begin branch March 17, 2025 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants