Add Notes and Flag API endpoints#23
Conversation
Visual Studio, why do you always automatically add incorrect usings when I paste code? Even when the code did not need any usings added?
| /// <param name="postIds">The IDs of one or more posts to check.</param> | ||
| /// <param name="activeOnly">Whether to fetch only active notes.</param> | ||
| /// <param name="limit">Optional note limit, default 300 (max 320). This is not the post limit!</param> | ||
| public Task<IList<Note>?> GetNotesAsync(IEnumerable<int> postIds, bool activeOnly = true, int limit = 320); |
There was a problem hiding this comment.
To be consistent with the rest of the library, it should be that activeOnly and limit are made nullable. For example, for pools the method looks like:
public Task<ICollection<Pool>> GetPoolsAsync(
int id,
Position position,
string? name = null,
string? description = null,
IEnumerable<int>? ids = null,
int? creatorId = null,
string? creatorName = null,
bool? isActive = null,
bool? isDeleted = null,
PoolCategory? category = null,
int? limit = null
);The same goes for the limit parameter in GetFlagsAsync.
There was a problem hiding this comment.
I actually don't agree with that.
Having the default value specified in the definition like this makes it easy to see at a glance when you are writing code that uses a function what the default values for the function parameters are. Making it default to null and needing to check in the function if the value is null before doing anything makes the code more complex and less readable but also makes it harder to tell at a glance what the default values are and making you have to read the summary (which can be several lines of text), if there even is a summary, which there currently isn't for every piece of code.
In some cases it makes sense for the default value to be null, if it being null causes a different code path to run. In this case we just want to pass a parameter to the API request regardless of whether the user provides one or not and defining the default values like this allows people to easily see at a glance what the default value is so they can decide if they need to override it or not.
The reason I'm saying that we want to pass a parameter regardless is because the defaults (at least for notes) are not reasonable. The default limit is the same as for posts, 75. But there can be 30 notes on a single post. That adds up pretty quick as the limit is per note and not per post, your code will quickly run into that limit and people may not even realize it's happening. That's why I've specifically set it pretty high by default.
As for is_active. It is not documented what the default behavior is if that field is not provided. This would be rather easy to test though and it seems likely that the default is already "true". In which case defaulting it to true in the code doesn't really do anything, but it does make the behavior a lot clearer to the user/developer. Not everybody wants to read the API documentation or test things themselves to figure out the behavior of something. We also can't assume that some of these API endpoints won't have the behavior suddenly change on the server side, breaking things on the client side because a default value changed. There is already one example where this behavior is not consistent on the server side. The is_active flag on pools in particular seems to not really do much. Because pools will show up on the website regardless of what the value of this is. You can specify it in API queries but it isn't obvious to most people that it defaults to false, when it defaults to true everywhere else, especially as the API is not that well documented.
The one thing I am not sure about now that I'm thinking about it is whether setting it to false causes active notes to be excluded. As I've named the variable activeOnly, that might be a misnomer. Needs more testing.
Scratch that bit. After testing it turns out setting it to false excludes active notes. Then the only way to include both active and inactive notes is to not specify the parameter at all (which warrants defaulting it to null in this case), which my code currently does not support. I'll fix that real quick.
For flags, there are usually only 0-3 for a single post. But you can pass multiple post IDs to fetch and there doesn't seem to be a limit to the number of post IDs you can pass as parameters, only to the number of results.
Most people ideally would probably want no limit for this at all, since they want every post ID they provide as parameters to have their flags/notes returned, but since the number of flags/notes varies, that isn't a guarantee, but setting the limit as high as possible makes it more likely and makes it easier to work with, as there is already a lot of guesswork in trying to figure out how many post IDs you can safely request without hitting the limit and having a lower (default) limit makes that problem worse.
That's my reasoning why having the defaults be like this is reasonable. They don't quite work like other API endpoints, since the limit isn't per post and the number of results isn't predictable.
If you insist on these changes, the single post GetNotesAsync and GetFlagsAsync would also need to have limit as an optional parameter, since it's theoretically possible that a single post can have over 75 notes or flags. Unlikely (but not impossible) for flags, but for notes it's quite possible, since I've already seen ~30 on one post in my limited testing.
And it would be pretty much mandatory for every user to manually specify a higher limit for GetNotes if it was not set to a high value by default, since pagination is not a thing on flags and notes so you can't easily just do multiple requests to get the remainder of the data. I needed to do a rather hacky workaround to get around that fact in my code, even with the limit at 320, reducing the number of post IDs and repeating the request until it didn't hit the limit, since there's no way to know which posts had notes omitted when the results aren't in a predictable order (they're ordered by note ID, but you can't know the note IDs ahead of time, and you can't request specific note IDs anyway)
Some of the other API endpoints could probably benefit from having default values clearly specified in the definition the same way I have done. If the default values (in the API endpoint) are not necessarily known to everyone using this library, we shouldn't expect everyone to read the e621 API documentation to find it. That is after all probably why they are using a library, to avoid having to do exactly that.
There was a problem hiding this comment.
Hmm, yeah I do get where you are coming from with that argument of "having the values specified makes it more clear what happens". I suppose I just wish e621's API documentation was also written in a way where you don't have to figure stuff out like you had to with the is_active parameter having different behaviors for null, true and false.
The limit for how many posts can you fetch when passing post IDs is probably first limited by the length of the URL it produces. I thought about leaving a comment about it, but after checking other endpoints, we don't do that other places either, so API consumers just shouldn't be doing anything too crazy :D
| using Newtonsoft.Json.Converters; | ||
| namespace Noppes.E621.Converters | ||
| { | ||
| // Custom converter for Rectangle |
There was a problem hiding this comment.
I don't like using Rectangle from System.Drawing here. That is more of a UI-related system library and using that in the API models is very odd. I'd prefer to just have a NotePosition class in E621Client. Then you also don't need this whole NotePositionConverter class because it's just a simple object with some getters and setters.
There was a problem hiding this comment.
The note JSON reply is a flat structure (1 dimensional), I couldn't find any way to tell Newtonsoft.Json that some fields in particular should be filled into a different class without using a custom converter, but for consistency with other parts of the code I thought it looked better that way. If we just place the x/y/width/height in the base Note class (like how they are laid out in the JSON) then we indeed don't need the converter, if you would prefer that.
As for the Rectangle I am actually not sure if System.Drawing is available in the open source/cross platform .NET base so it could be good to change it anyway just to ensure cross platform compatibility.
Rectangle does provide some useful extra functionality, like .Size and .Location and .Right and .Bottom as well as some functions, which if people needed any of that extra functionality they would have to create a Rectangle anyway so it could save a step. But I understand your concern, and I've changed it.
Not sure if you would prefer me to place the x/y/width/height in the base Note class instead, that would simplify the code but might not be as pretty. I guess there's no right or wrong answer either way, it's just personal preference.
There was a problem hiding this comment.
Not sure if you would prefer me to place the x/y/width/height in the base Note class instead, that would simplify the code but might not be as pretty. I guess there's no right or wrong answer either way, it's just personal preference.
I'd prefer placing the x/y/width/height in the base Note class yes! As all developers, I've got some regrets with this library, and being too opinionated with the models of the API is one of them. So I'd prefer to just map this 1-to-1 as the e621 response.
| /// Retrieves flags for one or more posts, with their descriptions | ||
| /// </summary> | ||
| /// <param name="postId">The ID of a post to check.</param> | ||
| public Task<IList<Flag>?> GetFlagsAsync(int postId); |
There was a problem hiding this comment.
The convention in E621Client is to return an ICollection instead of an IList. If there's a good reason to change it, we can of course. But if there's no reason then I'd like this to also be ICollection for a consistent interface.
There was a problem hiding this comment.
I'd actually been changing things in my fork to return IList instead of ICollection but I didn't include that in the pull requests because it wasn't a necessary change.
Kinda completely forgot I'd used it in the code I wrote, though.
There's no good reason, really. It just makes things a little more convenient, not having to do .ToList() to access certain functions/extension methods, but it's such a minor thing.
On a large scale, needing to call .ToList() rather than have them already be lists will increase the time it takes code to run but I don't really see this being a notable problem, the API requests are always going to be the main bottleneck (and at least in my case it isn't a frequent occurrence where I need those extra methods that IList offers)
| using Newtonsoft.Json.Converters; | ||
| namespace Noppes.E621.Converters | ||
| { | ||
| // Custom converter for Rectangle |
There was a problem hiding this comment.
Not sure if you would prefer me to place the x/y/width/height in the base Note class instead, that would simplify the code but might not be as pretty. I guess there's no right or wrong answer either way, it's just personal preference.
I'd prefer placing the x/y/width/height in the base Note class yes! As all developers, I've got some regrets with this library, and being too opinionated with the models of the API is one of them. So I'd prefer to just map this 1-to-1 as the e621 response.
|
2 participants
Notes are primarily for translations, these get overlaid on top of images on the website so for feature parity with the website it's useful to have these and without them a lot of context is often missing.
Flags are displayed on a post's webpage, often these tell you why a deleted post was deleted or warn you that a post is about to be deleted but it can be other things. Again, this allows for feature parity with the website.
Between these two additions, I believe every (public) API endpoint has been implemented. There are certainly things that aren't fully implemented, for example there are additional optional parameters on many of the API endpoints that aren't currently implemented, which could be something to look into in the future, though they are not often needed so they are less important.