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.

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

Add optional create collection params #29

Merged
merged 3 commits into from
Aug 5, 2023
Merged

Conversation

StefMa
Copy link
Collaborator

@StefMa StefMa commented Aug 1, 2023

Closes #27

Follows the same pattern as in #20

@StefMa StefMa requested a review from rsjethani August 1, 2023 04:05
collections.go Outdated
Comment on lines 144 to 147
func newCollectionsCreateClient(sl *sling.Sling, params collectionsCreateParams) *CollectionsCreateClient {
copy := sl.New()
copy.Post(common.CollectionsCreateEndpoint()).BodyJSON(&data)
return &CollectionsCreateClient{sl: copy, params: params}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

second parameter should be name only and while returning the new CollectionsCreateClient give it a new param with name field initialized from the name parameter

Copy link
Collaborator Author

@StefMa StefMa Aug 5, 2023

Choose a reason for hiding this comment

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

Fixed here bee03ff

Question:
Shuld this then be changed too?

go-outline/documents.go

Lines 81 to 85 in 7256b80

func newDocumentsCreateClient(sl *sling.Sling, params documentsCreateParams) *DocumentsCreateClient {
copy := sl.New()
return &DocumentsCreateClient{sl: copy, params: params}
}

(In another PR of course 😁 )

collections.go Outdated Show resolved Hide resolved
collections.go Outdated Show resolved Hide resolved
@StefMa StefMa requested a review from rsjethani August 5, 2023 09:05
@StefMa
Copy link
Collaborator Author

StefMa commented Aug 5, 2023

@rsjethani in case you are fine with my changes, you can approve it directly.
I will squash and merge then with a proper commit message 🙂

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.

LGTM, a rebase is needed of course.

@StefMa StefMa merged commit b2ae556 into main Aug 5, 2023
3 checks passed
@StefMa StefMa deleted the create-collection-optional branch August 5, 2023 17:26
@StefMa StefMa mentioned this pull request Aug 7, 2023
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.

Support for optional properties for creating a collection
2 participants