-
Notifications
You must be signed in to change notification settings - Fork 0
initial implementation #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
54c66cc to
667ea59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 🔥 🔥
please update the description with the issue, adding tests also would be good
|
|
||
| ### Building the CLI | ||
|
|
||
| Executing `make build-client` will create a binary named `timetracker` in the `bin` directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to unify the name timetracker or timetracker-cli
it's timetracker-cli in makefile
also:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Makefile
Outdated
| go build -o $(CLIENT_BIN) $(CLIENT_DIR)/main.go | ||
|
|
||
| # Format code using go fmt | ||
| fmt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can add .golangci.yml file and enable whatever linters you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
| # Run server | ||
| run-server: build-server | ||
| @echo "Running server..." | ||
| $(SERVER_BIN) -config=./team-timetracker-server.sample.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to run but got got error Binary was compiled with 'CGO_ENABLED=0', go-sqlite3 requires cgo to work.
I think we should add CGO_ENABLED=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added that in the Makefile now.
README.md
Outdated
| #### Start time entry | ||
|
|
||
| ``` | ||
| ~> ./bin/timetracker start github.com/theissues/issue/142 "helllooo starting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the commands but it requires the config file for each one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it falls back to ~/.config/team-timetracker.json if the file doesn't exist, or that didn't work?
| return nil, fmt.Errorf("error reading config file: %w", err) | ||
| } | ||
|
|
||
| var cfg Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add validations, for example only sqlite driver is supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check for sqlite, not sure if there's anything else considerable beyond valid filenames/filepaths.
| func (s *Server) setupRoutes() { | ||
| // Tracking routes | ||
| s.Router.HandleFunc("/api/start", s.TrackingHandler.StartTracking).Methods("POST") | ||
| s.Router.HandleFunc("/api/stop", s.TrackingHandler.StopTracking).Methods("POST") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why using POST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you suggest?
…lf given it doesn't support error wrapping directives
…Start action and ignored the Encode Result given that it should not fail
…ted for the start and in the api client check
28413d7 to
546b42f
Compare
No description provided.