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

gofumpt lint and misc cleanup #1682

Merged
merged 2 commits into from
Apr 20, 2024
Merged

Conversation

Limero
Copy link
Contributor

@Limero Limero commented Apr 7, 2024

This runs the gofumpt linter over the code base and then some additional manual cleanups

@@ -326,7 +326,7 @@ func TestGetFileExtension(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
if got := getFileExtension(fakeFileInfo{test.fileName, test.isDir}); got != test.expectedExtension {
t.Errorf("at input %q expected %q but got %q", test.fileName, test.expectedExtension, got)
t.Errorf("at input '%s' expected '%s' but got '%s'", test.fileName, test.expectedExtension, got)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

%q quotes strings with double quotes, so changing to '%s' to match output of other tests

ui.go Outdated Show resolved Hide resolved
@@ -471,7 +471,7 @@ func (win *win) printDir(ui *ui, dir *dir, context *dirContext, dirStyle *dirSty
filename = append(filename, []rune(gOpts.truncatechar)...)
filename = append(filename, lastPart...)
}
for i := runeSliceWidth(filename); i < maxFilenameWidth; i++ {
for j := runeSliceWidth(filename); j < maxFilenameWidth; j++ {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i is used in parent for, so this shadowed it

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup.

Going forward I'm not sure how to enforce the use of gofumpt, or whether it is necessary at all. On one hand it is nice to have stricter linting, but on the other hand it is extra effort to setup for contributors. Unless you are happy to submit this kind of PR once in a while.

ui.go Outdated Show resolved Hide resolved
@Limero
Copy link
Contributor Author

Limero commented Apr 8, 2024

Thanks for the cleanup.

Going forward I'm not sure how to enforce the use of gofumpt, or whether it is necessary at all. On one hand it is nice to have stricter linting, but on the other hand it is extra effort to setup for contributors. Unless you are happy to submit this kind of PR once in a while.

We could run it in a pipeline, but I think just seeing this as a one-off cleanup is fine. I did this to fix the inconsistencies caused by my previous MR, mostly with octal integers, like 0755 being formatted as 0o755. New contributors won't be confused with seeing different formats and just match the format of the rest of the code.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks once again for the changes

@gokcehan gokcehan merged commit a018257 into gokcehan:master Apr 20, 2024
4 checks passed
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.

3 participants