Skip to content
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

Implement documents.update #40

Merged
merged 7 commits into from
Aug 21, 2023
Merged

Implement documents.update #40

merged 7 commits into from
Aug 21, 2023

Conversation

zadjadr
Copy link
Contributor

@zadjadr zadjadr commented Aug 7, 2023

Closes #38

@StefMa StefMa changed the title Implement documents.update Implement documents.update Aug 7, 2023
StefMa
StefMa reviewed Aug 7, 2023
View reviewed changes
package_test.go Outdated Show resolved Hide resolved
StefMa
StefMa requested changes Aug 7, 2023
View reviewed changes
Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Overall the code loks good 👍

documents.go Outdated Show resolved Hide resolved
documents.go Show resolved Hide resolved
package_test.go Outdated Show resolved Hide resolved
package_test.go Outdated Show resolved Hide resolved
package_test.go Outdated Show resolved Hide resolved
StefMa
StefMa previously approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

LGTM 👌
Thanks!

@rsj-ioki
Copy link
Collaborator

@zadjadr please rebase ;)

rsjethani
rsjethani requested changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@rsjethani rsjethani left a comment

Choose a reason for hiding this comment

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

Please add the _failed test also, otherwise just some cosmetic changes

documents.go Outdated
@@ -78,16 +78,16 @@ type DocumentsCreateClient struct {
params documentsCreateParams
}

func newDocumentsCreateClient(sl *sling.Sling, params documentsCreateParams) *DocumentsCreateClient {
func newDocumentsCreateClient(sl *sling.Sling, title string, collectionId CollectionID) *DocumentsCreateClient {
Copy link
Collaborator

@rsjethani rsjethani Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

collectionId -> id; the type shows what the param is representing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here d87d1c4

documents.go Outdated
Comment on lines 159 to 163
// Update returns a client for updating a single document in the specified collection.
// API reference: https://www.getoutline.com/developers#tag/Documents/paths/~1documents.update/post
func (cl *DocumentsClient) Update(id DocumentID) *DocumentsUpdateClient {
return newDocumentsUpdateClient(cl.sl, id)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this above, with other DocumentsClient methods

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here a3077af

package_test.go Outdated
@@ -427,7 +486,7 @@ const exampleCollectionsListResponse_1collection string = `
}
}`

const exampleDocumentsCreateResponse_1documents string = `{
const exampleDocumentsResponse string = `{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Documents -> Document

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here 527e858

@StefMa
Copy link
Collaborator

StefMa commented Aug 21, 2023

Please add the _failed test

Fixed here befbbf0

@StefMa StefMa requested a review from rsjethani August 21, 2023 06:04
StefMa
StefMa approved these changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator

@StefMa StefMa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I should have approved this PR, not only commenting 😁

However, LGTM 👌

@zadjadr @rsjethani
I did the requested changes to finnaly get this thing merged.
Please have a look 🚀
(The whole thing will be of course rebased in case you approve 😉 )

@zadjadr
Copy link
Contributor Author

zadjadr commented Aug 21, 2023

I should have approved this PR, not only commenting 😁

However, LGTM 👌

@zadjadr @rsjethani I did the requested changes to finnaly get this thing merged. Please have a look 🚀 (The whole thing will be of course rebased in case you approve 😉 )

Thanks @StefMa, sorry had a pretty packed week! LGTM 👍

rsjethani
rsjethani approved these changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator

@rsjethani rsjethani left a comment

Choose a reason for hiding this comment

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

Thank you guys!

@rsjethani rsjethani merged commit 274d75a into ioki-mobility:main Aug 21, 2023
3 checks passed
@StefMa StefMa mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsjethani rsjethani rsjethani approved these changes

@StefMa StefMa StefMa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement documents.update API
4 participants
@zadjadr @rsj-ioki @StefMa @rsjethani