Skip to content

Conversation

dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Jan 26, 2018

supersedes #16
depends on #43
partially solves #3

This PR prepares the skeleton of the backend api serving mocks, using the requirements defined by #3, and following the agreements on the implementations as we talked irl

If approved, the things that will be done in the next PRs will be:

  • using DB stuff,
  • tests,
  • using user from Auth

@dpordomingo dpordomingo mentioned this pull request Jan 26, 2018
type errObj struct {
Title string `json:"title"`
}
// TODO: it should be private
Copy link
Contributor

Choose a reason for hiding this comment

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

imo, don't need to be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it private because nobody from the outside of the package should write in the response; should it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can easily imagine when we would use it.
For example, we would create an endpoint to download diff or file.

func GetFile(r, w) {
  name = getFileName(r)
  f, err := getFile(name)
  if err != nil {
    Write(w, Error{"file with name %s doesn't exist", name})
  }
  http.ServeContent(w, f)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but... that endpoint should be a handler, right?
If so, it can use handler.write too, as done by handler.OauthCallback
76ed472#diff-3ad2ebeb7ccf861129aa87b26fa1f1d8R31

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhhh. you moved it from serializer to handler. then all good!

func (e httpError) Error() string {
if msg := e.Title; msg != "" {
return msg
} else if msg = http.StatusText(e.Status); msg != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

for the future: you don't need to use else if you return before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done to reuse the msg implicit definition made by the first if.
Anyhow, there is no necessity of reusing it, so I'll separate it into two different if, and redefine the msg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done by 39208c3

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

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

I saw few issues (for example about logger), but we can fix it later. Good to go!!!!!

@dpordomingo
Copy link
Contributor Author

Sorry @smacker and @carlosms for making you review this again, but I had to address the comments:

  • Convert some internals to private things:
    • 821153d and : handler.assignmentRequest,
    • 76ed472 handler.write and serializer.countResponse
  • Cosmetics:
    • 39208c3 Separate if/return/else as suggested by @smacker
    • c97bdb7 Cosmetics: Run go-vet. Rename id>ID and unkeyed structs

Once you approve that last changes, I'll squash and merge. Thanks

Copy link
Contributor

@bzz bzz left a comment

Choose a reason for hiding this comment

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

LGTM.

Awesome job @dpordomingo!

Shall we rebase on latest master and merge?

@bzz bzz mentioned this pull request Jan 29, 2018
8 tasks
@carlosms
Copy link
Contributor

LGTM

@dpordomingo dpordomingo force-pushed the new-router-structure branch 2 times, most recently from c9a3151 to 9983b3f Compare January 29, 2018 20:01
@dpordomingo
Copy link
Contributor Author

I rebased 9983b3f but I couldn't merge because this PR depends on #43

I also added 4f445cd with github.com/rs/coors dependency as required by ./server/router.go

Added after PR review:
- Naming conventions as suggested by @smacker
- Separate if/return/else as suggested by @smacker
- Move assignmentRequest to the handler that uses it
- Make the handler.write and serializer.countResponse private
- Avoid nil serializer.Response
- go-vet. Rename id>ID
- go-vet. Avoid unkeyed structs
@dpordomingo dpordomingo merged commit 6a081b7 into src-d:master Jan 30, 2018
@dpordomingo dpordomingo deleted the new-router-structure branch January 30, 2018 11:57
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.

4 participants