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.

Sign up for GitHub

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

Jump to bottom

fix: General fixes before public release. #9

Merged
merged 5 commits into from
Nov 12, 2021
Merged

fix: General fixes before public release. #9

merged 5 commits into from
Nov 12, 2021

Conversation

danmrichards
Copy link
Contributor

@danmrichards danmrichards commented Nov 12, 2021 •
edited
Loading

  • Tidies up error handling on the internal event loop
  • Fixes type repetition on game.New func
  • Added clarifying comment on internal processor readiness
  • Event and InternalEvent are now type declarations instead of aliases (which are not meant for everyday use)
  • Adds comments on event types and consts
  • Export Event consts and add comments
  • Query packages now return concrete structs instead of interface, "accept interfaces, return structs"
  • Fixes error types that did not conform to the XXXError naming convention
  • Adds some suggestions in TODOs
  • Restructures application to place publicly usable code in the pkg directory.
  • Adds SIGTERM to signal handler and an explanation on its usage.
  • Refactors UDP binding to use a done channel instead of atomics
  • Removes internalEvent type.
  • Refactors internal event processor to use a done channel instead of capturing and replaying internal event on shutdown.
  • TCP server now echos packets back to the client.

fix: General fixes before public release.
bd67f56 
- Tidies up error handling on the internal event loop
- Fixes type repetition on game.New func
- Added clarifying comment on internal processor readiness
- Event and InternalEvent are now type declarations instead of aliases (which are not meant for everyday use)
- Adds comments on event types and consts
- Export Event consts and add comments
- Query packages now return concrete structs instead of interface, "accept interfaces, return structs"
- Fixes error types that did not conform to the XXXError naming convention
- Adds some suggestions in TODOs
danmrichards
danmrichards commented Nov 12, 2021
View reviewed changes
simple-game-server-go/pkg/game/game_loop.go
@@ -14,18 +14,30 @@ import (
)

type (
EventType = int
InternalEvent = int
Copy link
Contributor Author

@danmrichards danmrichards Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

See https://yourbasic.org/golang/type-alias/ and https://golang.org/doc/go1.9#language

danmrichards
danmrichards commented Nov 12, 2021
View reviewed changes
simple-game-server-go/pkg/proto/a2s/a2s.go
@@ -40,7 +41,7 @@ var (

// NewQueryResponder returns creates a new responder capable of responding
// to a2s-formatted queries.
func NewQueryResponder(state *proto.QueryState) (proto.QueryResponder, error) {
func NewQueryResponder(state *proto.QueryState) (*QueryResponder, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, it's good practice to accept interfaces and return structs. Interfaces are there to assert behaviour on a given type, which is generally in the domain of the thing accepting the type. Also, it makes this package easier to test, particularly if it had internal methods not part of that interface.

simple-game-server-go/pkg/proto/a2s/errors.go
// ErrUnsupportedQuery is an error which represents an invalid SQP query header.
ErrUnsupportedQuery struct {
// UnsupportedQueryError is an error which represents an invalid SQP query header.
UnsupportedQueryError struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/golang/go/wiki/Errors#naming

@danmrichards danmrichards marked this pull request as ready for review November 12, 2021 11:27
@danmrichards danmrichards requested a review from a team as a code owner November 12, 2021 11:27
- Restructures application to place publicly usable code in the pkg d…
f856474 
…irectory.

- Adds SIGTERM to signal handler and an explanation on its usage.
- Refactors UDP binding to use a done channel instead of atomics
- Removes internalEvent type.
-Refactors internal event processor to use a done channel instead of capturing and replaying internal event on shutdown.
- TCP server now echos packets back to the client.
nweedon-u
nweedon-u reviewed Nov 12, 2021
View reviewed changes
Copy link
Collaborator

@nweedon-u nweedon-u left a comment

Choose a reason for hiding this comment

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

Looks good, one nit - would loadConfig do better in the config package now as something like NewConfigFromFile()?

Fixes potential double close of udpBinding.
2dcc3aa 
Moves configuration loading to the public config package
@danmrichards
Copy link
Contributor Author

Looks good, one nit - would loadConfig do better in the config package now as something like NewConfigFromFile()?

Good shout, done

@nweedon-u nweedon-u merged commit 5eedec8 into main Nov 12, 2021
@nweedon-u nweedon-u deleted the untracked/pre-public-review branch November 12, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nweedon-u nweedon-u nweedon-u approved these changes

@milonoir milonoir milonoir approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danmrichards @milonoir @nweedon-u