-
Notifications
You must be signed in to change notification settings - Fork 4
Added test code + fetch route #4
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mathisco-01, overall good work. Only a couple of minor things that need to be reviewed.
Both the "TEST_INTERVAL" and "PORT" constants are not being used throughout the source code.
Outside of this PR, we should consider replacing the occurrences of "localhost" with a constant placeholder to facilitate distributing this application in a cloud environment.
api/src/bump_test.go
Outdated
"guildId": guildId, | ||
}) | ||
|
||
resp, _ := http.Post("http://localhost:8080/V1/bump", "application/json", bytes.NewBuffer(reqBody)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
api/src/main.go
Outdated
const ( | ||
PORT = ":8080" | ||
BUMP_INTERVAL = 60 // 1 minute in seconds | ||
TEST_INTERVAL = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constant has not been implemented/used.
I almost forgot to mention but I added documentation for the fetch route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mathisco-01, good job on resolving the issues. Looking forward, we should add more commenting to the source code, as this will increase readability and maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Added some questionable (but working) test code.
You can run it by doing
go test
.Also added the
/V1/fetch
route to get the latest 10 bumped servers.