[Core] Restructure repo to follow industry golang standards by Gustavobelfort · Pull Request #412 · pokt-network/pocket

Gustavobelfort · 2022-12-20T16:09:26Z

Description

This refactors the codebase to follow the standards outlined in https://github.com/golang-standards/project-layout.
Additionally some changes on the Makefile were applied to make it mode readable and maintainable.

Issue

Fixes #379

Type of change

Please mark the relevant option(s):

List of changes

Refactor codebase to follow golang project layout standards
QOL changes to make the Makefile more readable and maintainable by splitting it into multiple domain oriented files
- The new sub makefiles are locates at ./scripts
Updated documentation pointing to internal packages
Updated golang codebase to take in account the new package location
Updated the protobuf files and its corresponding scripts to build correctly with the new package paths

Testing

make develop_test
LocalNet w/ all of the steps outlined in the README

Required Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have tested my changes using the available tooling
I have updated the corresponding CHANGELOG

If Applicable Checklist

I have updated the corresponding README(s); local and/or global
I have added tests that prove my fix is effective or that my feature works
I have added, or updated, mermaid.js diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

Olshansk

Definitely a hard change to review.

I'm not the biggest fan of everything but open to having the discussion.

Left some comments around naming decisions.
We started committing mocked files and generated files that we did not before
When I tried to run the tests it did not work for me. Did it pass for you?
This library isn't exactly an SDK, so is there really a reason to have 90% of the codebase under internal just because that's what the best practices say? Have you looked at how other blockchain Go projects (Cosmos, Tendermint, Flow, Celestia, et al) do it?

Olshansk · 2023-01-03T19:59:51Z

Makefile

 .PHONY: client_connect
 client_connect: docker_check ## Connect to the running client debugging daemon
-	docker exec -it client /bin/bash -c "go run -tags=debug app/client/*.go debug"
+	docker exec -it client /bin/bash -c "go run -tags=debug cmd/p1/*.go debug"


I'm not so sure about renaming app/client to cmd/p1.

Is this the golang standard?

p1 is the binary name we selected, but what if we choose to rename it? E.g. after v0 is deprecated and v1 is live, we might name it pocket instead. I felt like client was general enough.

Olshansk · 2023-01-03T20:00:23Z

Makefile

@@ -1,22 +1,11 @@
-include build.mk
+include scripts/build.mk


Big fan of this but just not that you might hit some merge conflicts soon. Shouldn't be an issue but just FYI

Olshansk · 2023-01-03T20:44:55Z

internal/rpc/client.gen.go

@@ -0,0 +1,546 @@
+// Package rpc provides primitives to interact with the openapi HTTP API.


Did we include this file in main before this change?

Olshansk · 2023-01-03T20:45:47Z

internal/rpc/handlers.go

-	"github.com/pokt-network/pocket/app"
-	"github.com/pokt-network/pocket/shared/codec"
-	typesUtil "github.com/pokt-network/pocket/utility/types"
+	app "github.com/pokt-network/pocket/cmd"


This seems a bit weird.

If app is a good semantic name for it, then I'd argue we shouldn't rename cmd to app.

Why the change?

Olshansk · 2023-01-03T20:46:22Z

internal/rpc/module.go

-var (
-	_ modules.RPCModule = &rpcModule{}
-)
+var _ modules.RPCModule = &rpcModule{}


Do you really think rpc should be under internal?

Olshansk · 2023-01-03T20:47:10Z

internal/shared/messaging/proto/events.proto

@@ -0,0 +1,7 @@
+syntax = "proto3";


Do you know why git didn't treat this as a mv while it did for most of the others?

Olshansk · 2023-01-03T20:48:15Z

internal/shared/modules/mocks/bus_module_mock.go

@@ -0,0 +1,188 @@
+// Code generated by MockGen. DO NOT EDIT.


Why did we start committing mocked files now?

We did not do so before and shouldn't start doing it now.

Olshansk · 2023-02-07T17:50:25Z

Summarised some comments here for whoever picks this up in the future.

initital folder structure

faffbb8

Gustavobelfort changed the title ~~FEAT: Restructure repo to follow industry golang standards~~ [WIP] Restructure repo to follow industry golang standards Dec 20, 2022

Gustavobelfort added 4 commits December 21, 2022 16:16

update makefile file references to the internal folder

fc3f2f0

update makefile script splitting

80c21da

checkou main config file

0343f28

refactor makefile

0b7ef61

Gustavobelfort self-assigned this Dec 26, 2022

Gustavobelfort added the core Core infrastructure - protocol related label Dec 26, 2022

Gustavobelfort added this to the M*: Pocket QoL (Quality of Life) milestone Dec 26, 2022

Gustavobelfort linked an issue Dec 26, 2022 that may be closed by this pull request

[Core] Follow industry standards for Golang project folder structure #379

Open

4 tasks

Gustavobelfort requested a review from Olshansk December 27, 2022 00:05

Gustavobelfort marked this pull request as ready for review December 27, 2022 00:06

Gustavobelfort changed the title ~~[WIP] Restructure repo to follow industry golang standards~~ [Core] Restructure repo to follow industry golang standards Dec 27, 2022

Olshansk added the code health Nice to have code improvement label Dec 28, 2022

Olshansk requested changes Jan 3, 2023

View reviewed changes

Olshansk mentioned this pull request Feb 7, 2023

[Core] Follow industry standards for Golang project folder structure #379

Open

4 tasks

Olshansk closed this Feb 7, 2023

Olshansk mentioned this pull request Mar 23, 2023

[Testing] Organizing test utilities package(s) #609

Closed

9 tasks

Olshansk deleted the feat/golang-standard-structure branch June 2, 2023 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Restructure repo to follow industry golang standards#412

[Core] Restructure repo to follow industry golang standards#412
Gustavobelfort wants to merge 5 commits intomainfrom
feat/golang-standard-structure

Gustavobelfort commented Dec 20, 2022 •

edited

Loading

Uh oh!

Olshansk left a comment

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk Jan 3, 2023

Uh oh!

Olshansk commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,546 @@
		// Package rpc provides primitives to interact with the openapi HTTP API.

Conversation

Gustavobelfort commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Gustavobelfort commented Dec 20, 2022 •

edited

Loading