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

Add golangci-lint configuration and appease some linters #13

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

silkeh
Copy link
Member

@silkeh silkeh commented Sep 1, 2023

This PR adds configuration and a GitHub workflow action for golangci-lint. Trivially fixed linting issues have been fixed, and other linters disabled so they can be enabled with a little more care..

Copy link
Member

@livingsilver94 livingsilver94 left a comment

Choose a reason for hiding this comment

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

Your linting PR was made against master, but I reworked the cli package in the relive branch, you should lint that branch instead.

cli/run.go Outdated Show resolved Hide resolved
cli/run.go Outdated Show resolved Hide resolved
@silkeh
Copy link
Member Author

silkeh commented Sep 2, 2023

Your linting PR was made against master, but I reworked the cli package in the relive branch, you should lint that branch instead.

I don't understand: this PR is a standalone change that has little to do with the CLI overhaul. As such, it should generally go into master.

@livingsilver94
Copy link
Member

You linted cli in master which was overhauled in relive, so your linting in that package is DOA. If you don't mind re-linting it after we've merged the branch, then it's all good 😄

@livingsilver94
Copy link
Member

livingsilver94 commented Sep 3, 2023

Should you not like the "temporary master branch" approach but prefer advancing the master in little steps, just lemme know and we'll go that way. My initial idea was to resume working on usysconf in relive and eventually merge it into master.

@silkeh
Copy link
Member Author

silkeh commented Sep 3, 2023

You linted cli in master which was overhauled in relive, so your linting in that package is DOA. If you don't mind re-linting it after we've merged the branch, then it's all good 😄

That's fine, these changes can wait 🙂

Should you not like the "temporary master branch" approach but prefer advancing the master in little steps, just lemme know and we'll go that way. My initial idea was to resume working on usysconf in relive and eventually merge it into master.

Personally, I would reserve the 'temporary master branch' (or 'long-lived feature branch') approach for when master is actively being developed and/or changes are likely to subtly break things. That doesn't seem the case here, so I would just advance master in little steps.

@silkeh
Copy link
Member Author

silkeh commented Sep 11, 2023

@livingsilver94: I have rebased on the latest master. GLHF 😉

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

Successfully merging this pull request may close these issues.

2 participants