Fix wrong commands path on Makefile #257
Conversation
|
Tried locally, Seems a bit confusing due to https://github.com/src-d/guide/blob/master/engineering/conventions-go.md#executables As this was not arch-specific issue, to avoid future 🐛 like this, do you think it might be worth to also include an "integration test" CI profile, that tried to launch binary after |
|
Shouldn't we instead change the folder name to |
ajnavarro
force-pushed
the
fix/makefile-cmd-to-cli-path
branch
4 times, most recently
from
2855338 to
01005db
Compare
cmd/gitbase/main.go
Outdated
| @@ -3,7 +3,7 @@ package main | |||
| import ( | |||
| "os" | |||
|
|
|||
| "github.com/src-d/gitbase/cli/gitbase/cmd" | |||
| "github.com/src-d/gitbase/cmd/gitbase/cmd" | |||
There was a problem hiding this comment.
maybe we should move the contents of the cmd folder into the gitbase folder. It does not make much sense to have them in a folder and the path looks weird
ajnavarro
force-pushed
the
fix/makefile-cmd-to-cli-path
branch
from
01005db to
d6abd51
Compare
|
@erizocosmico done. |
cmd/gitbase/server.go
Outdated
| @@ -1,4 +1,4 @@ | |||
| package cmd | |||
| package main | |||
There was a problem hiding this comment.
Why? Subcommnads are supposed to be implemented in a subpackage.
There was a problem hiding this comment.
Why do they need to be in a subpackage?
There was a problem hiding this comment.
@mcuadros What was the rationale to separate subcommands on a different package? Allowing imports?
There was a problem hiding this comment.
Testing. The commands should be tested, usually we don't test main packages, but the logic of the commands should be tested, so having the command in a separate package, leaves more clear that should be testested, also is cleaner.
Additionally allow more complex patterns, as is used in other projects were several commands are sharing the same commands.
|
Who this PR should the problem of the build? |
|
@mcuadros Packfile had a wrong path pointing to the commands folder. |
|
But this PR is not changing the Makefile |
|
That was the first approach, but we decide to do it correctly and change instead the folder to the correct one due to the company guidelines: |
|
But this PR is removing the package for the commands. |
Signed-off-by: Antonio Jesus Navarro Perez <[email protected]>
ajnavarro
force-pushed
the
fix/makefile-cmd-to-cli-path
branch
from
d6abd51 to
c0a3abb
Compare
|
Moved all commands to |
|
Merging to fix the build. If we want to add more changes to the path, we can do it into another PR. |
Signed-off-by: Antonio Jesus Navarro Perez [email protected]