noppesthefolf / e621client Goto Github PK

Blazor on WebAssembly implements just parts of the functionality the HttpClientHandler class is expected to have. Part of this due to certain technical limitations the browser brings with it, the other part is because the Blazor team removed features they considered bloat in order to cut down the size of the assemblies that have to be shipped to the browser. Blazor is at the time of writing still in preview, but is expected to get a stable release in May 2020. It’d be nice to be able to support Blazor on WebAssembly in order to enable the development of PWAs using this library.

The library appears to be hardcoded to only accept up to 6 tags in searches, but this does not seem to be an official API limitation. In fact, the API docs state "Any tag combination that works on the website will work here." and the website certainly does allow more than 6 tags. Is this limit arbitrary or am I missing something?

Adding an optional CancellationToken to each request, will allow it to be canceled. This shouldn’t be too much work and becomes especially interesting in combination with async enumeration.

I ran into an issue when using your package to interact with the E621 API. I wanted to unit test the code that makes use of the E621Client

I have no real code examples to show but it boils down to this:
Because of the builder / factory pattern the constructor of E621Client is set to internal because of which a mocking framework cannot create a mocked instance of the client.

It would be a possibility to solve this by creating a IE621Client interface that gets returned by the builder so that we can mock on that interface.

I will try to implement this and will link a PR to this issue when it's ready.

Some endpoints can be navigated using pagination or some other method. Adding methods that harness the power of the new IAsyncEnumerable introduced in C# 8, could be very convenient.

Noticed these two are in the JSON data, but not exposed in the data types.
Interestingly enough, they aren't documented on the wiki either.

Simple change in Post.cs:

        /// <summary>
        /// Whether the post has notes associated with it.
        /// </summary>
        [JsonProperty("has_notes")]
        public bool HasNotes { get; set; }

        /// <summary>
        /// The duration of the post, if it is a video, or null otherwise
        /// </summary>
        [JsonProperty("duration")]
        public double? Duration { get; set; }

IqdbPost.cs is a bit more complicated. has_notes is not there, but we have another thing instead which we can use which does not exist in normal posts, namely last_noted_at:

        [JsonProperty("last_noted_at")]
        public DateTimeOffset? LastNotedAt;

        [JsonProperty("duration")]
        public double? Duration { get; set; }

And in AsPost():

                HasNotes = LastNotedAt != null,
                Duration = Duration

I've tested these changes and they seem to work fine and the behavior corresponds with what is expected. There is the possibility of a post having a note added and later deleted, resulting in last_noted_at being set even though the post does not currently have any notes, so IqdbPost wrongly assumes the post has notes. This is probably fairly rare though and not a high priority issue. It might not even be an issue, if last_noted_at is simply fetched from the latest note added rather than being a fixed field in the database.

On a side note, it is a shame that some of the post information is not available through the API.
Things like uploader name, deleted by, deleted reason, notes. Notes are used for translations and the usefulness of the others is self explanatory. These are not available through the database exports either (presumably they live in separate databases which are not exported)

Edit: Noticed Duration is there in DbExportPost, but gets converted to TimeSpan. So for consistency, best to do the same here, I suppose.

When using multiple tags, for example "domestic_cat" and "black_fur", separated by space the E621Client.GetPostsAsync method returns an empty list. It is however possible to search both tags separately and get results back.

E621 allows you to set this when doing an IQDB search on their site, in my testing so far it seems like a value >= 80 means some correct matches are not returned. A value below 60 definitely returns some false positives. Around 75 seems to be a good value. The default when nothing is specified seems to be higher (90?) so some correct matches would not be returned. So being able to specify the cutoff would be quite useful.