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

Update ephemeral-roles.go #85

Merged
merged 3 commits into from Jan 12, 2018
Merged

Update ephemeral-roles.go #85

merged 3 commits into from Jan 12, 2018

Conversation

Aareon
Copy link
Contributor

@Aareon Aareon commented Jan 10, 2018

I hope this helps in some way. My main goal was to remove as much duplicate code as possible without breaking the thing. I also wanted to try to remove at least one unnecessary function call if need be. Please request any changes as need be, I’m quite new to Golang. Thanks for the awesome bot!

I hope this helps in some way. My main goal was to remove as much duplicate code as possible without breaking the thing. I also wanted to try to remove at least one unnecessary function call if need be. Please request any changes as need be, I’m quite new to Golang. Thanks for the awesome bot!
@ewohltman
Copy link
Owner

Thanks very much for the PR! I'll take a closer look and get back to you with any feedback ASAP!

@Aareon
Copy link
Contributor Author

Aareon commented Jan 10, 2018

I’m not sure if that error is my doing, or if it’s the nature of the script, but if there is something that needs changing, I will gladly do so.

Edit: This was my doing, I forgot to pass envVar to log.Fatalf in main()

My last commit had quite a few errors that I missed. This commit corrects most if not all, and improves what I broke in the conditional in `monitorGuildsUpdate` in the last commit.
@ewohltman
Copy link
Owner

ewohltman commented Jan 11, 2018

Looks good to me, great catch/idea with replacing the environment variable lookups with a slice of strings!

If I could request one last minor update: if we're going to move from explicitly closing the dgBotSession to deferring it, could we move the defer to right after the Open() error check?

	// Open the websocket and begin listening
	err = dgBotSession.Open()
	if err != nil {
		log.WithError(err).Fatalf("Error opening Discord session")
	}
	defer dgBotSession.Close()

I'd be happy to merge right in with that, thanks!

@Aareon
Copy link
Contributor Author

Aareon commented Jan 11, 2018

Absolutely! I’d be happy to polish this off tomorrow, I’m a bit sick today. If you have any other requests for changes, please let me know. I’m kind of learning as I Go 😉

Move deferring dgBotSession.Close() to just after opening the session for readability purposes.
@Aareon
Copy link
Contributor Author

Aareon commented Jan 11, 2018

I’m concerned now about how you feel about my usage of the switch-case in this scenario. It may be a bit superfluous. My reasoning behind using it was almost exactly that. I did figure that instead of doing len(dgBotSession.State.Guilds as many times as it’s done now, it may be better to store it and reuse it.

On another note, it does add a smidge more garbage. So that is something to consider.

@ewohltman
Copy link
Owner

Since it's only an extra int, I'm not concerned about the extra "garbage" from a new, meaningful variable 😄

If I am not mistaken, because it is a primitive data type (allocated onto stack instead of heap?) scoped within the for loop, each time the loop block completes the memory allocated for the int is freed because it is no longer in scope so there would be no garbage to collect (and maybe the same block in memory is re-allocated on the next iteration, or we get it from somewhere else).

Otherwise, merging! 🎉

@ewohltman ewohltman merged commit 8157583 into ewohltman:master Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants