feat(v3): Warning for nonexistent mocks #911
Conversation
There was a problem hiding this comment.
Just a couple of tweaks and this should be good to go. Thanks for the PR!
Regarding your comment about the length of .Run(), I don't necessarily believe that functions need to be split up because of line counts. My philosophy is that you should break it up for any of the following reasons:
- To make unit testing easier.
- To factor out common bits of logic used in more than one place (or in other words, abstracting complex bits of logic out).
- To make it easier to understand what is happening.
There are little to no unit tests for this method because I'm relying much more heavily on e2e integration tests, so point 1 is moot. There is only one consumer of this logic, so point 2 is moot. And IMO the method is fairly easy to understand so point 3 is moot. I would prefer to have all the core business logic in one large-ish function if it makes it easier to follow the flow of the logic. Abstractions can sometimes run the risk of making it harder to know what's actually happening.
I understand this is just a preference, but from a practical perspective, I don't see a reason to break it up. But, I appreciate the perspective!
internal/cmd/mockery.go
Outdated
| @@ -322,5 +356,16 @@ func (r *RootApp) Run() error { | |||
| } | |||
| } | |||
|
|
|||
| // The loop above could exit early, so sometimes warnings won't be shown | |||
| // until other errors are fixed | |||
| for packageName := range missingMap { | |||
There was a problem hiding this comment.
| for packageName := range missingMap { | |
| for packagePath := range missingMap { |
Technically, a "package name" is different from a "package path".
internal/cmd/mockery.go
Outdated
| log.Warn(). | ||
| Str(logging.LogKeyInterface, iface). | ||
| Str(logging.LogKeyPackagePath, packageName). | ||
| Msg("no such interface") |
There was a problem hiding this comment.
| Msg("no such interface") | |
| Msg("interface not found in source") |
Just to be a bit more explicit.
|
Just pushed the requested changed, I hope it's good now. Also thanks for such a detailed answer on |
Description
Copy of #907 but for v3. Also, no
log.Ctx()this time :DType of change
Version of Go used when building/testing:
How Has This Been Tested?
Checklist
On a side note, don’t you think that RootApp.Run is a little too big? Having 2 (or 3 now) huge loops with all the core logic feels like a bit too much. Just my two cents as an outside contributor.
I've also put my code in there, but looks like it could become a mess with more features