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 collection support #201

Open
wants to merge 7 commits into
base: 2.0-.Net-Core
Choose a base branch
from

Conversation

chasedog
Copy link
Contributor

Adds support for:

/api/v1/collections/add_post_to_collection
/api/v1/collections/collection
/api/v1/collections/create_collection
/api/v1/collections/delete_collection
/api/v1/collections/subreddit_collections

To-do:
/api/v1/collections/remove_post_in_collection
/api/v1/collections/follow_collection
/api/v1/collections/reorder_collection
/api/v1/collections/update_collection_description
/api/v1/collections/update_collection_display_layout
/api/v1/collections/update_collection_title

This branch is based on chasedog:fix-build-and-update-packages. I will rebase once that's merged.

  - Upgrade to net core 2.2
  - Remove deprecated Microsoft.Extensions.SecretManager.Tools
  - Remove unnecessary service 82a7f48d-3b50-4b1e-b82e-3ada8210c358
  - Add IsTestProject flag to test project
@@ -8,6 +9,8 @@ namespace RedditSharp
[Serializable]
public class RedditException : Exception
{
public JArray Errors { get; }

Copy link
Owner

Choose a reason for hiding this comment

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

Does this actually return the errors from Reddit properly? That's been something I've been meaning to fix up. It also needs an HTTP status code preferably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does, but only for the new endpoints I've implemented. But yeah, I agree the code in ExecuteRequestAsync needs to be cleaned up to expose the errors instead of swallowing an exception.

Example response when trying to delete a collection that doesn't exist: {"json": {"errors": [["INVALID_COLLECTION_ID", "That collection doesn't exist", "collection_id"]]}} but the status code is 200.

"errors" is an array of arrays... kinda weird, but the consumer can do whatever with it

Copy link
Contributor

Choose a reason for hiding this comment

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

@CrustyJew It appears RedditHttpException has the status code built in. Perhaps eventually having an Error class that can be used across the board?

Copy link
Owner

Choose a reason for hiding this comment

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

@chasedog @jkrejcha I think it would be better to get these rolled together so you have one exception type and can determine if it is an HTTP status code issue or a reddit result issue.

Copy link
Contributor

@jkrejcha jkrejcha Aug 29, 2019

Choose a reason for hiding this comment

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

The great thing about the reddit API is that it seems to be a combination of both, either one, none, or sometimes a completely different error code dependent on whether you're accessing via the site or via the JSON API. There does need to be handling of errors of some sort. RedditHttpException should probably inherit from RedditException IMO but it's hard to say.


namespace RedditSharp.Interfaces
{
internal interface ISettableWebAgent
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose behind having this as an interface here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for PopulateObjects to have a way to set the web agent for each object. So when you fetch the collections, you can do collection operations (e.g. AddPostAsync) directly to the object instead of passing in the web agent, for example.
Do you prefer the web agents to be set after populating the objects here:

var result = Helpers.PopulateObjects<Collection>(json, WebAgent);
foreach (var collection in result)
    collection.WebAgent = WebAgent;
return result;

Or pass in Action<T> (e.g. x => x.WebAgent = WebAgent) that gets called after the item is populated?

Or something else?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm thinking it may make more sense just to make the WebAgent settable by default instead of having a specific interface for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't PopulateObjects just use the Thing's static Parse() method instead of creating a new interface?

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.

3 participants