feat: refactor to use interfaces #35
Conversation
Go test coverage: 2.7% for commit c64e257View coverage for all packages# Package Name | Coverage
+ sublinks/sublinks-federation/internal/lemmy | 2.7% |
| } | ||
| defer conn.Close() |
There was a problem hiding this comment.
We should keep this one and remove the one on line 29. Better to call defer early
internal/http/activity.go
Outdated
| if err != nil { | ||
| log.Error("Error reading object", err) | ||
| s.Error("Error reading object", err) |
There was a problem hiding this comment.
Maybe it's just me, but this feels really weird (essentially calling server.Error doesn't translate to "log an error")
There was a problem hiding this comment.
Sounds good since I'm embedding the Logger interface I can either call s or s.Logger
Modifying it
internal/http/post.go
Outdated
| func SetupPostRoutes(r *mux.Router) { | ||
| r.HandleFunc("/post/{postId}", getPostHandler).Methods("GET") | ||
| func (s Server) SetupPostRoutes() { | ||
| s.HandleFunc("/post/{postId}", s.getPostHandler).Methods("GET") |
There was a problem hiding this comment.
The same thing here as the logging. server.HandleFunc just doesn't make as much sense. Maybe if it was server.AddRoute? When I see server.HandleFunc I think of adding a middleware, not a route
internal/log/log.go
Outdated
| var body interface{} | ||
| rawbody, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| Error("Error reading request body", err) | ||
| l.Error("Error reading request body", err) |
There was a problem hiding this comment.
On some of the methods where they are only a single line or two, I'm kind of ok with short variable names. However, as we get into longer functions it makes more sense to use more descriptive names. If we just want to make things simpler and say "always use descriptive variable names" I'd be fine with it also.
internal/log/log.go
Outdated
| } | ||
|
|
||
| func Info(msg string) { | ||
| func (l Log) Info(msg string) { | ||
| log.Info().Msg(msg) |
There was a problem hiding this comment.
Shouldn't these all be updated to l instead of log?
There was a problem hiding this comment.
Yep - gone ahead and updated this. The interface methods have the same names as the embedded zerolog package so its kind of verbose ie. logger.Logger.Info()
|
Made updates! |
cmd/federation.go
Outdated
| conn, err := db.Connect() | ||
| defer conn.Close() |
There was a problem hiding this comment.
Hmm... I think the other line I commented on is better no? Shouldn't we check whether an error occurred before trying to defer closing? (not sure if the Fatal logger will still call deferred functions and cause more errors?)
internal/http/activity.go
Outdated
| } | ||
|
|
||
| func getActivityHandler(w http.ResponseWriter, r *http.Request) { | ||
| func (s Server) getActivityHandler(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Maybe we can compromise and use srv for these? I really don't like non-descriptive variable names unless they are used in a for loop as an iterator (eg for i = 0; i < 5; i++ type stuff) or in methods with only a couple of lines (so it's easy enough to keep track of what the variable means. Even then, it would probably be better if we standardized and used better naming everywhere. I know I'm guilty of this with the request/response writer variables)
There was a problem hiding this comment.
Oh yes absolutely. I know go devs tend to use single character vars but I much prefer verbosity. I'll change all the single char vars to "server"
internal/http/activity.go
Outdated
| @@ -44,7 +42,7 @@ func getActivityHandler(w http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| break | |||
| default: | |||
| error.Error(fmt.Errorf("action %s not found", vars["action"])) | |||
| error.Error(fmt.Errorf("action %server not found", vars["action"])) | |||
There was a problem hiding this comment.
I'm guessing you searched and replaced? This one should stay as %s.
There was a problem hiding this comment.
yep yep! just changed back
RFC: