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 linter warnings and add GH lint workflow #1074

Merged
merged 2 commits into from
Aug 28, 2023
Merged

fix linter warnings and add GH lint workflow #1074

merged 2 commits into from
Aug 28, 2023

Conversation

syhpoon
Copy link
Contributor

@syhpoon syhpoon commented Aug 24, 2023

No description provided.

@syhpoon syhpoon requested review from sesposito and zyro August 24, 2023 17:39
Copy link
Member

@sesposito sesposito left a comment

Choose a reason for hiding this comment

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

I've left some comments questioning a couple of linter default rules, but if they cannot be changed or it begins too many discussions around personal preferences, then I'm in favor of just sticking with the defaults 👍

@@ -2202,7 +2201,7 @@ func (n *RuntimeGoNakamaModule) LeaderboardCreate(ctx context.Context, id string
return errors.New("expects a leaderboard ID string")
}

sort := LeaderboardSortOrderDescending
var sort int
Copy link
Member

@sesposito sesposito Aug 28, 2023

Choose a reason for hiding this comment

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

Personally I prefer the use of the short assignment operator syntax here (and in other places) as it explicitly conveys the semantic meaning of the default zero value, can the linter be configured to ignore these? WDYT?

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'd say that in this case the problem is that the initial assignment is not very useful since it gets completely overridden in the following switch statement. To your point, the same clarity can be achieved by introducing an enum type alias so that's it's clear it's not a plain integer value here but an enum. Would this solve your concern, Simon?

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 also in favour of the extra readability. Even if the value is overridden shortly afterwards it's an indicator that this should be the "base" case. If there's another way to achieve that I'm open to that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was thinking of something like this:

type LeaderboardSort = int

const (
	LeaderboardSortOrderAscending LeaderboardSort = iota
	LeaderboardSortOrderDescending
)

...

var sort LeaderboardSort
	switch sortOrder {
...

this way it's clear that we're dealing with enum and not a generic int, plus, given that the switch below is exhaustive, we don't really need a base case. In short, imho there's no loss of readability with this approach, but, if the consensus is to keep it as it was, I'll be happy to throw some annotations instead 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I reverted the "base-case" implementation and added annotations to ignore these.

presenceObj.Set("sessionId", e.Presence.SessionId)
presenceObj.Set("username", e.Presence.Username)
presenceObj.Set("node", e.Presence.Node)
_ = presenceObj.Set("userId", e.Presence.UserId)
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 a bit on the fence on this rule, I understand the value it may bring in preventing unhandled errors, but otherwise, omitting the return value assignment is much cleaner when that's the intent, and having to explicity ignore this around defer statements seems cumbersome.

Copy link
Contributor Author

@syhpoon syhpoon Aug 28, 2023

Choose a reason for hiding this comment

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

I guess it's more of a personal preference here, I'd personally always prefer to explicitly show that I willingly ignore the returned error and not just forgot to handle it. I agree about the defer though, I'll take a look if it's possible to configure the linter to avoid having to annotate those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a rule to ignore the cleanup function. By default it actually ignores quite a lot of stdlib functions in defer so this was the only custom edge case.

@syhpoon syhpoon merged commit 577907a into master Aug 28, 2023
2 checks passed
@syhpoon syhpoon deleted the lint branch August 28, 2023 20:43
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