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

Jcastets/linter #317

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

Jcastets/linter #317

wants to merge 54 commits into from

Conversation

brmzkw
Copy link
Contributor

@brmzkw brmzkw commented Jan 27, 2025

The goal of this PR is not to fix the lint issues, but instead to add the linter to the CI to improve the quality of the future code.

We will probably need to spend some time to review what errors have been silenced and should have been handled instead.

caching/store.go Outdated Show resolved Hide resolved
cmd/plakar/subcommands/exec/exec.go Show resolved Hide resolved
snapshot/archive.go Show resolved Hide resolved
snapshot/archive.go Show resolved Hide resolved
snapshot/exporter/fs/fs.go Outdated Show resolved Hide resolved
@brmzkw
Copy link
Contributor Author

brmzkw commented Jan 28, 2025

To recap the discussion of this morning on Discord, I'll do the following:

1/ update this PR to regroup fixes in different commits. For example, there will be one commit for _ = flag.Parse, one commit for another type of error, and so on.
2/ I will remove the command find which is causing linting issues, and will not be included in the beta.
3/ The PR can then be merged to main.
4/ Then, I'll create an issue with the following content: "The commit silenced the linter. We need to review whether silencing was the right thing to do, if we should instead do something about it (eg. bubble up the error), or if we should update the linter configuration to ignore these errors and instead revert the commit". This issue will have to be reviewed to improve the current code.

@brmzkw brmzkw force-pushed the jcastets/linter branch 9 times, most recently from 1e6e6ec to 95888d7 Compare February 4, 2025 10:19
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.

2 participants