Skip to content

Comments

Backend: REST API docs#38

Merged
dpordomingo merged 1 commit intosrc-d:masterfrom
dpordomingo:documentation
Jan 30, 2018
Merged

Backend: REST API docs#38
dpordomingo merged 1 commit intosrc-d:masterfrom
dpordomingo:documentation

Conversation

@dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Jan 26, 2018 •
edited
Loading

Depends on #37 and #43

Documents the backend public things → only the last commits: 468f689, 6431393, 468f689

carlosms
carlosms approved these changes Jan 29, 2018
View reviewed changes
Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

Approved, with minor comments

server/handler/assignments.go

// GetAssignmentsForUserExperiment returns the experiments for the logged user and a given experiment
// if these assignments does not already exist, they are created in advance
func GetAssignmentsForUserExperiment() RequestProcessFunc {
Copy link
Contributor

@carlosms carlosms Jan 29, 2018

Choose a reason for hiding this comment

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

Instead of saying it returns something, should it be "returns a function that..."?
Also, do you mean returns the assignments? And "these assignments do not"

server/handler/render.go Outdated
w.Write(content)
}

// RequestProcessFunc is the function that takes a http.Request, and returns a serializer.Response and an error
Copy link
Contributor

@carlosms carlosms Jan 29, 2018

Choose a reason for hiding this comment

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

an http

server/repository/repositories.go

// ErrNoAssignmentsInitialized is the error returned when are requested the Assignments of a User
// for a given Experiment, but they has not been yet created
var ErrNoAssignmentsInitialized = fmt.Errorf("No assignments initialized")
Copy link
Contributor

@carlosms carlosms Jan 29, 2018

Choose a reason for hiding this comment

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

is the error returned when the Assignments of a User are requested
for a given Experiment, but they have not been yet created

server/repository/users.go Outdated

func (r *Users) Get(id int) (*model.User, error) {
// Get returns the User identified by the passed ID.
// If the user does not exists, if returns no user nor error
Copy link
Contributor

@carlosms carlosms Jan 29, 2018

Choose a reason for hiding this comment

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

does not exist,

@bzz bzz changed the title Documentation Documentation of REST API Jan 29, 2018
@dpordomingo dpordomingo force-pushed the documentation branch 2 times, most recently from 003d739 to 468f689 Compare January 29, 2018 20:33
@dpordomingo
Copy link
Contributor Author

dpordomingo commented Jan 29, 2018

I rebased over other PR dependencies, and added only the following commits:

  • 6431393 Explain better how handler.RequestProcessFunc and handler.Get behaves
  • 468f689 Fix grammar issues as spotted by @carlosms

Thanks for your review!

@dpordomingo dpordomingo removed the request for review from smacker January 29, 2018 20:38
bzz
bzz approved these changes Jan 30, 2018
View reviewed changes
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.

@dpordomingo Great job! Thank you for addressing all feedback fast.

In this case though, it's not clear what is the motivation to have a documentation update in a different PR than #37.

In my experience, having doc update at the same PR where new features/code is added (may be as a separate commit) simplifies both, review and merge processes.

@bzz bzz changed the title Documentation of REST API Backend: REST API docs Jan 30, 2018
@smacker
Copy link
Contributor

smacker commented Jan 30, 2018

it was my suggestion to make a separate PR to avoid blocking #37 in case there will be many comments.

@dpordomingo
Document public stuff
697a5d7 
From PR review:
- Explain better the routing arq as requested by @carlosms
- Fix grammar issues as spotted by @carlosms
@dpordomingo dpordomingo merged commit 4857901 into src-d:master Jan 30, 2018
@smacker
Copy link
Contributor

smacker commented Jan 30, 2018

@dpordomingo I remember I left a comment about the name of wrapper Get. Don't see it now most probably because of rebase. But also don't see any answer for it.

@dpordomingo dpordomingo mentioned this pull request Jan 30, 2018
Closed
@dpordomingo
Copy link
Contributor Author

dpordomingo commented Jan 30, 2018

I also tried to look for it too because I also remembered it, but I could not find so I just disregarded it 🤕

Thanks for reviewing it again. I'd suggest continuing with it in a separated issue #46

@dpordomingo dpordomingo deleted the documentation branch January 30, 2018 14:14
dpordomingo pushed a commit that referenced this pull request Mar 26, 2018
@mcuadros
Merge pull request #38 from src-d/feature/test-with-race
8e0b7be 
test go code with -race
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@carlosms carlosms carlosms approved these changes

@bzz bzz bzz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dpordomingo dpordomingo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dpordomingo @smacker @carlosms @bzz