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

fix: enable more linters #234

Merged
merged 3 commits into from
Jan 3, 2025
Merged

fix: enable more linters #234

merged 3 commits into from
Jan 3, 2025

Conversation

nobe4
Copy link
Owner

@nobe4 nobe4 commented Jan 3, 2025

No description provided.

@nobe4 nobe4 merged commit 6419a6f into main Jan 3, 2025
7 checks passed
@nobe4 nobe4 deleted the multiple-linters branch January 3, 2025 11:19
internal/actions/print/print.go Show resolved Hide resolved
if err := os.MkdirAll(filepath.Dir(c.path), 0o755); err != nil {
if err := os.MkdirAll(
filepath.Dir(c.path),
syscall.S_IRUSR|syscall.S_IWUSR|syscall.S_IXUSR,
Copy link
Contributor

Choose a reason for hiding this comment

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

What linter suggested you to use this?

I have never met this constants

Copy link
Owner Author

Choose a reason for hiding this comment

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

It was mnd, for the "magic number" 755 and 6xx in other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a new rule for this

sashamelentyev/usestdlibvars#101

Thank you for letting me know it

Copy link
Owner Author

Choose a reason for hiding this comment

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

Amazing, this will be very useful!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if you saw it, but my linter suggestion went in a dead end

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I noticed. I might switch to x/sys, but as long as no linter complains about it I'll leave it 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I add myself a depguard rules about using syscall package to enforce using golang.orgg/x/sys

Copy link
Owner Author

Choose a reason for hiding this comment

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

When you do, feel free to ping me, I'll make the same change :)

Copy link
Contributor

@ccoVeille ccoVeille Jan 9, 2025

Choose a reason for hiding this comment

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

Copy link
Owner Author

@nobe4 nobe4 Jan 9, 2025

Choose a reason for hiding this comment

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

fyi, I decided to move back to using numbers with an updated mnd rule: #240

internal/cmd/config.go Show resolved Hide resolved
internal/cmd/root.go Show resolved Hide resolved
internal/notifications/view.go Show resolved Hide resolved
Comment on lines +21 to 26
Read(d any) error
Write(d any) error
Refresh(t time.Time)
RefreshedAt() time.Time
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Did a linter suggested you to add variable name here? If so, which one?

I'm curious about adding it to my own settings

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I will take a look

nobe4 added a commit that referenced this pull request Jan 9, 2025
Using syscall is not recommended and having fixed numbers + the
appropriate mnd configuration is enough.

cc #234 (comment)
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