Skip to content

Backend: REST API docs #38

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 30, 2018
Merged

Backend: REST API docs #38

merged 1 commit into from
Jan 30, 2018

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

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

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

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

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

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

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