collection: Support collecting posts #14
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds support for deleting collections. Resolves #1.
Rather than accepting a naked string, accept a DeleteCollectionParams struct so that new optional parameters can be added in the future without breaking the API.
This adds support for moving posts to a collection. This change relies on #13 so that we can create and delete collections during tests. Couple notes: - CollectPosts accepts a single struct rather than an alias and a list of structs. This makes it easy to add new optional parameters in the future. - We're passing `[]*CollectPost` around rather than `*[]CollectPost` which is done in some of the existing APIs. The reasoning for this is: - Idiomatically, slices are passed by-value (`[]foo`) rather than by-pointer (`*[]foo`) because slices are already reference types. Plus they're really cheap to copy since they're just a triple: pointer, length, and capacity ([related blog post][1]). We need pointers to slices only when we're trying to modify the original slice reference and don't have the ability to return a slice, like with `json.Unmarshal` (see also [this post][2]). - If we're trying to reduce copying, `[]*foo` is better than `[]foo` because otherwise `foo` will be copied when manipulating or accessing entries in the slice (like ranging over it). Existing APIs were left as-is to avoid breaking them. I can switch to `*[]CollectPost` if you'd prefer that but `[]*CollectPost` is more idiomatic. [1]: https://blog.golang.org/go-slices-usage-and-internals#TOC_4. [2]: https://blog.golang.org/slices#TOC_5. Resolves #4.
ghost
suggested changes
May 27, 2019
There was a problem hiding this comment.
Hey @abhinav, thanks for this.
If your still interested in contributing this change there are a couple things we should change.
- the parameters don't really need to be structs, for DeleteCollection and CollectPost
- collection: Add support for deletion #13 has been merged
- there are a few merge conflicts from recent changes to the test file, or related to collection: Add support for deletion #13 merging ( I haven't checked yet )
| endpoint := "/collections/" + sp.Alias + "/collect" | ||
|
|
||
| var p []*CollectPostResult | ||
| env, err := c.post(endpoint, sp.Posts, &p) |
There was a problem hiding this comment.
no need to pass the address here as slices are reference types already
There was a problem hiding this comment.
You need to pass a pointer for the JSON unmarshaler to fill it with a decoded value. So json.Unmarshal requires the result argument to always be a pointer.
|
I want to hold off on anymore changes to this for now, as I implemented the same thing locally and would like to combine the best of both implementations. |
|
ok 👍 should we close this then? |
|
@thebaer @robjloranger Feel free to close this PR. |
|
we can reopen if needed |
ghost
closed this
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This PR includes changes from #13 because the change being
proposed here relies on functionality added in #13. The change itself
can be reviewed in its own commit. The commit message for it is
repeated below.
This adds support for moving posts to a collection. This change relies
on #13 so that we can create and delete collections during tests.
Couple notes:
CollectPosts accepts a single struct rather than an alias and a list
of structs. This makes it easy to add new optional parameters in the
future.
We're passing
[]*CollectPostaround rather than*[]CollectPostwhich is done in some of the existing APIs. The reasoning for this is
Idiomatically, slices are passed by-value (
[]foo) rather thanby-pointer (
*[]foo) because slices are already reference types.Plus they're really cheap to copy since they're just a triple:
pointer, length, and capacity (related blog post). We need
pointers to slices only when we're trying to modify the original
slice reference and don't have the ability to return a slice, like
with
json.Unmarshal(see also this post).If we're trying to reduce copying,
[]*foois better than[]foobecause otherwise
foowill be copied when manipulating oraccessing entries in the slice (like ranging over it).
Existing APIs were left as-is to avoid breaking them. I can switch to
*[]CollectPostif you'd prefer that but[]*CollectPostis moreidiomatic.
Resolves #4.