Skip to content

initial implementation#2

Open
xmonader wants to merge 11 commits intomasterfrom
development
Open

initial implementation#2
xmonader wants to merge 11 commits intomasterfrom
development

Conversation

@xmonader
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown

@rawdaGastan rawdaGastan left a 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

Comment thread README.md

### Building the CLI

Executing `make build-client` will create a binary named `timetracker` in the `bin` directory
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment thread Makefile Outdated
go build -o $(CLIENT_BIN) $(CLIENT_DIR)/main.go

# Format code using go fmt
fmt:
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Comment thread Makefile
# Run server
run-server: build-server
@echo "Running server..."
$(SERVER_BIN) -config=./team-timetracker-server.sample.json
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator Author

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.

Comment thread README.md Outdated
#### Start time entry

```
~> ./bin/timetracker start github.com/theissues/issue/142 "helllooo starting"
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator Author

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?

Comment thread cmd/server/main.go
Comment thread internal/server/config.go
return nil, fmt.Errorf("error reading config file: %w", err)
}

var cfg Config
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator Author

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.

Comment thread internal/server/handlers/tracking.go Outdated
Comment thread internal/server/handlers/tracking.go Outdated
Comment thread internal/server/server.go
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")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why using POST?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you suggest?

Comment thread internal/server/middlewares/cors.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants