Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions services/repository/files/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import (
)

func TestCleanUploadFileName(t *testing.T) {
assert.Empty(t, CleanGitTreePath(""))
assert.Empty(t, CleanGitTreePath("."))
assert.Equal(t, "", CleanGitTreePath("")) //nolint:testifylint // for readability and alignment
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, adding comments make it harder to read ...

I can help to rewrite the code to something like

for testCase := range cases {
    assert.Equal(....)
}

(if you don't mind)

Then we can get rid of the "nolint" tricks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go ahead, the maintainer edit thing is checked :)

Copy link
Member

@silverwind silverwind Jun 25, 2025

Choose a reason for hiding this comment

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

Yes, eliminating nolints is better. I think something ought to be done about the migration packages too. Maybe just a config in .golangci.yml that disables revive for those files, but it's a pretty broad disable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are fixable with a rename. I don't suppose that's breaking as migrations are internal and I'd be surprised if something in our code relied on package name. It's more of an issue of what name should there be.
There's also not much benefit in that and the _ rule could be suppressed globally (to cover oauth_* as well) or for migrations only.
See text based rules which I think would work for this

Copy link
Member

@silverwind silverwind Jun 25, 2025

Choose a reason for hiding this comment

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

Which specific revive rule is being triggered by these underscores? Maybe we can disable the rule itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

So why do you think we can't disable var-naming exactly?

See: But we can't fully disable it, because it also check other places like ID in models

There are too many unclear parts.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the ID topic is about, but then let's go with the skip-package-name-checks option?

Copy link
Contributor

@wxiaoguang wxiaoguang Jun 26, 2025

Choose a reason for hiding this comment

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

but then let's go with the skip-package-name-checks option?

Agree

Copy link
Contributor Author

@TheFox0x7 TheFox0x7 Jun 26, 2025

Choose a reason for hiding this comment

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

Done. Added comments in 80b05b7 to document why those 3 rules are in place.
I'll probably follow this PR up with a comment enforcement - I think the reason for disabling the rule will be very useful long term.


follow up is ready - no clue how to stack stuff here so I'll wait for this to land.

Copy link
Member

@silverwind silverwind Jun 27, 2025

Choose a reason for hiding this comment

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

You could go onto this branch and git cherry-pick <hash> the commit hash(es) from the other branch, but let's do the merge then.

assert.Equal(t, "", CleanGitTreePath(".")) //nolint:testifylint // for readability and alignment
assert.Equal(t, "a/b", CleanGitTreePath("a/b"))
assert.Empty(t, CleanGitTreePath(".git/b"))
assert.Empty(t, CleanGitTreePath("a/.git"))
assert.Equal(t, "", CleanGitTreePath(".git/b")) //nolint:testifylint // for readability and alignment
assert.Equal(t, "", CleanGitTreePath("a/.git")) //nolint:testifylint // for readability and alignment
}
2 changes: 1 addition & 1 deletion services/user/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func UpdateUser(ctx context.Context, u *user_model.User, opts *UpdateOptions) er
} else if !user_model.IsLastAdminUser(ctx, u) /* not the last admin */ {
u.IsAdmin = opts.IsAdmin.Value().FieldValue // it's safe to change it from false to true (not the last admin)
cols = append(cols, "is_admin")
} else /* IsAdmin=false but this is the last admin user */ { //nolint:gocritic // could flatten the condition, no idea how to flatten comments at the moment
} else /* IsAdmin=false but this is the last admin user */ { //nolint:gocritic // make it easier to read
if !opts.IsAdmin.Value().FromSync {
return user_model.ErrDeleteLastAdminUser{UID: u.ID}
}
Expand Down