Skip to content
This repository has been archived by the owner on Mar 12, 2023. It is now read-only.

Project Restructure #78

Merged
merged 11 commits into from
Jan 8, 2022

Conversation

RageCage64
Copy link
Contributor

The number 1 thing I wanted to accomplish with this restructuring is to co-locate relevant code. We had a discussion about it further up about a month and a half ago, when I think robotmayo posted an open source google project that showed this style. I've since seen it a number of times as well. The server package could maybe be moved to internals as well.

With this, the new idea is to have each package sort of represent one data model in our API, i.e. users games runs what have you. In each package:

  • The model definition and Store interface
  • Route definitions
  • Whatever store implementations (aka the data access)
  • The tests, which will make actual web requests to a gin engine with the routes for the package and use whichever store is chosen (this gets rid of the concern on my other open PR about not using the real database in tests)

The tests I have here are much better than the open PR. They use helpers in a nicer way, make use of t.Parallel() and are table based. They also use the real database connection.

to rebase is to feel pain

this is going away too

added tests, added local store

i'm gonna rebase
@RageCage64
Copy link
Contributor Author

Gonna have to figure out CI cause we need a test environment but don't track a normal .env file

Comment on lines +49 to +50
// Maybe we shouldn't use the increment ID but generate a UUID instead to avoid
// exposing the amount of users registered in the database.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we're doing UUIDs if we can. Issue for the future :)

@zysim
Copy link
Contributor

zysim commented Dec 17, 2021

I dig this structure. Maybe when I'm more awake tomorrow I'll also try figuring out how to get tests to work, if you don't already do by then. Then I'd like to see this merged :)

Copy link
Contributor

@FrozenSake FrozenSake left a comment

Choose a reason for hiding this comment

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

I like this article on structuring Go Code. I'm not generally a big fan of "common" because I think it means that code architecture hasn't really been properly planned out. It feels like a dumping ground for "stuff I need multiple places, but organized my other packages poorly."

Further, I don't understand why ping_test exists but we shoved all the database testing inside database.go

I'd like to see tests on server and users instead of just saying "it's hard", though, or at least a testing plan and issues/tasks drawn up for creating them.

(Also to reiterate: this is an improvement, I think this is good.)

Moved the stuff from common package into dedicated spots. DataStore
interface moved to database, SuccessResponse stuff moved into a new
package called request.
The original purpose of the package, to serve as an example for other
package layouts, is largely obsoleted by the existence of the users
package.
When I did the project-wide rename of the functions in the database
package, I didn't save the files that had stuff renamed.
Copy link
Contributor

@FrozenSake FrozenSake 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, and issues for the gaps to close later.

@RageCage64 RageCage64 merged commit 13fd0b2 into leaderboardsgg:main Jan 8, 2022
zysim pushed a commit to zysim/leaderboard-backend-go that referenced this pull request Jun 28, 2022
* Fix yaml syntax

* we actually need the .env file

* Rename solution

We're not a POC anymore!
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants