Align method classes with API structure in the official docs about bigbone HOT 31 CLOSED

Yes, a structure like that sounds nice. I could give this issue a go tomorrow, unless you're already working on it by then?

from bigbone.

I can do all of that this week - my concern was more that we would establish a completely new API just some days before release. On the other hand, that's what major releases are for, so I can do the whole work in one PR but separate commits and then we'll see if we want to go the whole way at once. :)

from bigbone.

@bocops Looking at your second code example:

StatusMethods st = client.statuses() is shorter

StatusMethods st = client.getStatusMethods() is more consistent in terms of naming.

Rethinking this a bit, it is probably not very Java-like to access a getter without the get prefix. Thought it might be a good idea to keep it short and sweet, but now I am a bit unsure about that.

But I am all-in for the <SingularTerm>Methods renaming.

from bigbone.

@bocops I am not worried about the overhead. I just try to (and it may be my personal code style) to refactor repeated client.statuses().* calls into a local variable and then using it, as I find it more clear that way. Obviously that is super subjective.

from bigbone.

It's two things

• the double plural is odd
• As said in #94 (comment) StatusesMethods st = client.getStatuses() feels wrong when reading, as getStatuses() implies for me that I get Status objects back. Not a method handle(r)

from bigbone.

I've had a quick look at how others are structuring their SDKs.

AWS SDK / Oracle Cloud SDK
They are building a separate client instance for each aspect of their platform (gateway, autoscaling, etc). For our use case, this seems overkill.

Twitter SDK
They're doing it pretty much in the same way as us. They have a client which exposes instances of certain classes via getters. In their case, classes are names BookmarksApi, ListsApi, etc... Their getters are called like client.bookmarks().xxx(), client.lists().xxs().

I think we're not doing it wrong if we go that route (leaving the get prefix away in the getter):

Java

// Example 1
var st = client.statuses();
st.postStatus(...)

// Example 2
client.statuses().postStatus(...);

Kotlin

// Example 1
val st = client.statuses
val request = st.postStatus(...)

// Example 2
val request = client.statuses.postStatus(...)

That looks ok to me.

from bigbone.

What if Methods gets replaced via Api as in StatusesApi ? For me API sounds better and we get rid of the double plural.

from bigbone.

I'm thinking of it less as caching data to avoid a repeated network request, and more as data that is integral to a specific client object

I see. Well, let's do it as proposed by you then. Should there be ever more functionality provideed in the Instance API, we can think about it again and do a major release then if something significantly needs to be changed.

Ok for you?

So to sum up, for the 2.0.0 release we:

• implement the <SingularTerm>Methods approach as discussed above and expose it to clients like client.statuses()
• implement the instance info as integral part of the client object and expose it via client.instance in Kotlin and via client.instance() in Java.

from bigbone.

Fine for me. Let's postpone this and just go with the singular term renaming then. 👍

from bigbone.

While we're doing that, we could also think about the exact names. Do we want to keep them as plurals to distinguish between entities (singular) and method classes working on them (plural), or can we find a better naming scheme than that?

I find it a bit hard to tell apart Instance and Instances at a glance, for example.

from bigbone.

Something obvious that comes to my mind is simply changing our method classes into something like this:
Accounts -> AccountsMethod
Search -> SearchMethod

More examples: ScheduledStatusesMethod, MutesMethod, FollowedTagsMethod, ...

I'll have a look at some other Mastodon client implementations and check how they do it.

from bigbone.

What actually irks me the most, now that I think about it, is the fact that functionality is spread across multiple classes. What we currently do is:

// create a client instance
val client = MastodonClient.Builder(instance).build()

// set up some method instance for a subset of functionality
val timelines = Timelines(client)

// perform some function on that instance
val foo = timelines.someTimelineMethod().execute()

// set up a different method instance for a different subset of functionality
val statuses = Statuses(client)

// perform some different function on that instance
val bar = statuses.someStatusMethod().execute()

Basically, we set up an instance of a client with very limited functionality itself that is already spread across MastodonClient and MastodonClient.Builder, and then we don't use that client to make calls, but instances of wholly different classes that combine somewhat arbitrary subsets of functionality - anything that happens to share the same REST API prefix, even if it does and/or returns wildly different things.

What we could do instead is either make MastodonClient an actual client that performs all that functionality itself:

// create a client instance
val client = MastodonClient.Builder(instance).build()

// perform some function on that instance
val foo = client.someTimelineMethod().execute()

// perform some different function on that instance
val bar = client.someStatusMethod().execute()

Or, if we want to keep functionality spread across different files for readability(?), we could make the current method classes method objects that at least don't need to be instantiated:

// create a client instance
val client = MastodonClient.Builder(instance).build()

// perform some function on that instance
val foo = Timelines.someTimelineMethod(client).execute()

// perform some different function on that instance
val bar = Statuses.someStatusMethod(client).execute()

from bigbone.

The only reason I see why someone would build a class structure like we have now is maybe:

• Readability / class size
• Testability eventually

But I agree, we could simplify this.

On the calling site, we could provide an API like you proposed in the first example but with additional members in the client class that kind of separates the methods logically, like

• client.statuses.postStatus()
• client.statuses.reblogStatus()
• client.timelines.blabla()

On implementation side, we still keep statuses, timelines, etc in separate classes. I think doing so would make it easier for the user to find the methods he needs (maybe even without reading the docs - which is a good thing I guess 😊).

Also, our classes would not explode in size, which is also nice.

from bigbone.

Feel free ro start working on it if you like 👍🏻 I'll do some other release prep stuff in the mean time 🙂

from bigbone.

I just started, and am already running into a decision to make. Which method classes exactly do we want to keep or newly create?

For example, we currently have a Blocks.kt, but in the Mastodon API documentation, "blocks" looks like an indented entry under "accounts". At the same time, there is an endpoint api/v1/blocks which returns Account, but also an endpoint api/v1/accounts/$id/block that performs a block and returns a Relationship.

I guess the underlying question is: which of these, sometimes mutually exclusive, concepts do we want to align our class structure with:

1. endpoint prefix, first level: everything under "api/vX/accounts" goes in Accounts, everything under "api/vX/blocks" goes in Blocks; independent of how it is documented, what function it performs and what it returns
2. API documentation, first indentation: everything under accounts goes in Accounts, including "bookmarks", "favourites", "mutes" and independent of their endpoint prefix
3. logical(?) grouping: for example, our Apps.kt currently combines api/v1/apps and oauth prefixes
4. by return value: for example, group all functionality that returns Account or lists thereof under Accounts.kt; this would be the most massive restructuring, so probably not at this point; it might also be a first step towards moving the whole functionality into respective entity classes
5. something else...?

What we currently do seems to be (1) with a bit of (3) sprinkled in - but we've already established that we want to refactor files like Public.kt, so pure (1) could be the goal. If we do that, we'd bloat our MastodonClient with roughly 40-50 new members to allow calls like client.statuses.postStatus() as suggested, though. I'm also unsure if a structure that has a different call for "get me all muted accounts" than for "get me all blocked accounts", just because they use different endpoints, will really be helpful for library users.

At the same time, there actually doesn't seem to be any rhyme or reason to the indentation in the API documentation. It seems as if this isn't actually meant to be a hierarchy but just highlighting "more important" functionality? So (2) is probably out of the question - and (4) as well, at least for an upcoming 2.0.0 release.

from bigbone.

I would probably go with 1. Putting everything which is under ../api/vX/blocks in a Blocks class. Hopefully, they've kept their APi methods listed in the API docs aligned with the prefixes they're using. 🙂

from bigbone.

From a quick poking around, that seems to be mostly the case - although I found some oddities like api/v1/notifications (plural) to get all notifications, versus api/v1/notification/$id (singular) to get a single one. If we strictly follow (1), that would mean a separate file for just the method that returns a single notification.

My suggestion would be that we skip the more aggressive rewrites of this conversation for 2.0.0, and talk about them again (for 3.0.0?). That would mean that I'll prepare a PR that shifts functionality around by endpoint prefix where necessary, but I won't at the same time:

• rename classes to AccountsMethods or similar
• introduce new accessor members in MastodonClient

from bigbone.

Ah, did not expect that. I would not be too strict in that case. /api/v1/notification and /api/v1/notifications could go into the same class I think, it would still be clear to the user, as those calls would reside in the notifications scope of MastodonClient, like client.notifications.xxx.

Everything in https://docs.joinmastodon.org/methods/accounts/ goes into client.accounts.xxx
Everything in https://docs.joinmastodon.org/methods/polls/ goes into client.polls.xxx
etc...
etc...
So we maintain a more or less 1:1 mapping.

It this is too much work, we can also move it to the 3.0.0. That's fine as well.

from bigbone.

@pilhuhn suggested on Mastodon that we could further refactor StatusesMethods statuses = client.getStatuses(); to StatusMethods st = client.getStatusMethods(); in Java.

I interpreted this as a request to rename the getters, which are currently auto-generated and are thus named after the members in Kotlin code, so the current name of the getter in Java is getStatuses() instead of the potentially more appropriate getStatusesMethods().

I now see that this also concerns the first term, the request seems to be to rename StatusesMethods to StatusMethods (and the getter to getStatusMethods()), and probably other of our method classes as well. Is that correct?

I agree that "StatusesMethods" is a somewhat awkward name, but just to make sure: plurals are what's used in Mastodon documentation in all of these cases. Do we still want to rename all XxxMethods classes to a singular first term?

from bigbone.

For me, classes such as StatusesMethods or StatusMethods are library-internal containers to group methods together that belong together, based on the Mastodon docs mostly. But I would not expose this name to the library user 1:1, because Method is not helpful to the user but clutter. Personally I would expose these classes to the user like this:

Java:

client.statuses().postStatus(...)

Kotlin:

client.statuses.postStatus(...)

In my opinion, it looks cleaner and is less typing. What do you think?

Regarding StatusesMethods vs. StatusMethods. This is an internal library thing. For code readability reasons I would probably rename StatusesMethods to StatusMethods, as simple as that. Just because it is more natural. I think it is still clear, that methods in this class are responsible for handling statuses.

Could you live with that? Or do you disagree? Happy to discuss further 🙂

from bigbone.

Wrt "StatusesMethods": Mastodon is using the plural and then has REST-API path like GET /api/v1/statuses/:id HTTP/1.1 so I understand the desire to keep the plural.
What puzzles me is the combination with 'Methods'. Especially as 'Methods' already has the plural. So +1 on my side to StatusMethods.

Or rename those classes to DealWithStatuses and DealWithTimelines and so on :-)

from bigbone.

Good point regarding the fact that the "Methods" suffix is just an artifact based on how the codebase is structured.

We're necessarily still exposing some of that structure to library users as long as it exists - and we can't completely prevent them from learning about the exact name of a class that must be public with the structure we have - but as long as they don't have to create any instances of the method classes themselves, that is probably fine. :)

@pilhuhn Regarding that last bit, the members of the MastodonClient that are exposed via getters in Java are initialized just once, lazily when they are requested the first time. This means that there probably shouldn't be any overhead of calling the getter multiple times, like:

client.statuses().methodA();
client.statuses().methodB();
client.statuses().methodC();

versus:

StatusMethods st = client.statuses();
st.methodA();
st.methodB();
st.methodC();

That at least avoids usage of the method classes' names in Java code of library users.

So, we'd do the following:

• rename method classes to <SingularTerm>Methods
• rename Java getters in MastodonClient to the same name as the underlying member (e.g. statuses(), instance(), timelines())

from bigbone.

Rethinking this a bit, it is probably not very Java-like to access getter without the get prefix.

This is true. :)

Taking a step back and playing devil's advocate: is the fact that we're going back and forth on what to name which part of this whole construct a code smell and perhaps a sign of other problems? Or is it really just the double plurals that is problematic?

from bigbone.

What we should definitely go for is for our Java and Kotlin APIs to be as close to each other as possible. If we use a get prefix in Java, but then also need to add a Methods suffix to clarify that this getter doesn't return individual entities already, there's not much similarity left between client.statuses in Kotlin and client.getStatusMethods() in Java. We should use either the long or the short name in both languages.

Simply using the "method group" name works in most cases, like statuses, timelines, accounts - but I see problems with instance, which is one of the few group names in Mastodon API that is singular instead of plural.

With the above design, client.instance would be the way to access whatever instance methods we have - e.g. client.instance.getInstance() - but it actually looks like a way to access the instance entity for the server that our client is built for.

Considering that there's only one instance method, returning the currently relevant server instance that probably shouldn't change for the lifetime of the client object we're building, and also considering that we already use that method to build our client, and already store parts of the relevant Instance in our client, like instance name and version, should we perhaps treat that as a special case and not make instance methods available here.

Instead, we could store the whole Instance object we already retrieved once while building the client, and then return that whenever necessary. Would that work for everyone?

from bigbone.

What we should definitely go for is for our Java and Kotlin APIs to be as close to each other as possible. If we use a get prefix in Java, but then also need to add a Methods suffix to clarify that this getter doesn't return individual entities already, there's not much similarity left between client.statuses in Kotlin and client.getStatusMethods() in Java. We should use either the long or the short name in both languages.

Agree.

With the above design, client.instance would be the way to access whatever instance methods we have - e.g. client.instance.getInstance() - but it actually looks like a way to access the instance entity for the server that our client is built for.

What about client.instances() - make it plural an be consistent in our naming (at least).

Considering that there's only one instance method, returning the currently relevant server instance that probably shouldn't change for the lifetime of the client object we're building, and also considering that we already use that method to build our client, and already store parts of the relevant Instance in our client, like instance name and version, should we perhaps treat that as a special case and not make instance methods available here.

Instead, we could store the whole Instance object we already retrieved once while building the client, and then return that whenever necessary. Would that work for everyone?

I see your point. For me it is hard to tell how the Mastodon API will evolve. I'd probably vote to maintain a separate InstanceMethods class and expose it via client.instances(). Whenever the Mastodon people decide to add more methods to the Instance API, we can just stimply add it there.

Fetching the full instance information during object initialization (using the builder) seems to cause troubles, as I would not really know where to put those additional methods.

Just as a side note: They way it works now, is that during builder instantiation, I just fetch the version attribute from the instance V1 or V2call. This is easy, as the attribute exists in both API versions.

I think the Mastodon API and the documentaton is not consistent in many places, but I think we should not carry those mistakes into our API. Just make it logical for the user to use the lib.

Happy to hear your thoughts on this.

from bigbone.

I like it. Names would then be like StatusesApi, BlocksApi, AccountsApi, InstancesApi, ...

from bigbone.

I think the Mastodon API and the documentaton is not consistent in many places, but I think we should not carry those mistakes into our API. Just make it logical for the user to use the lib.

I think the most logical thing from the perspective of a library user would be to not have to care about doing server requests (here client.instances().getInstance()) that only return the same, invariable information, some of which I shouldn't even have to care about, like the version. I understand the argument regarding potential new methods on that endpoint, though, so let's go with instances() and revisit at a later point if necessary.

Regarding XxxMethods vs. XxxApi, I thought about that as well after reading about the Twitter SDK example above. One complication would be that Mastodon already defines an "API" consisting of "Entities" and "Methods". If we now call our method-equivalents APIs, but still have entities on the same hierarchy level, as part of an overarching API, wouldn't that just be more confusing instead of less?

from bigbone.

I think the most logical thing from the perspective of a library user would be to not have to care about doing server requests (here client.instances().getInstance()) that only return the same, invariable information, some of which I shouldn't even have to care about, like the version. I understand the argument regarding potential new methods on that endpoint, though, so let's go with instances() and revisit at a later point if necessary.

Just to make sure I get your point: Your proposal was to call the /api/v1/instance and/or /api/v2/instance endpoint during client initialization and store the full response in the MastodonClient object for the lifetime of the client object. Then, let client.instances().getInstance() return that info to the caller (as we already have it). So no extra network request here.

But what about a scenario like this (ok, might be a bit constructed but still):
A user is observing instance data (like user count, etc...) by polling the instance. If he wants to get fresh instance data, he needs to re-instantiate our client, as he has no option of invoking a call to /api/v1/instance and/or /api/v2/instance himself.

I think it would be good, if the user has the option to request instance data whenever he likes, no?

Please correct me if I am wrong....

Regarding Methods vs Api: Fine for me if we go with the <SingularTerm>Methods approach... I've read an article about client SDK development. They said, good clients should align with the REST API as close as possible. I guess that also applies to names. To make sure, users are not confused and find stuff quickly. So yes, <SingularTerm>Methods might be better in the end.

from bigbone.

Just to make sure I get your point: Your proposal was to call the /api/v1/instance and/or /api/v2/instance endpoint during client initialization and store the full response in the MastodonClient object for the lifetime of the client object. Then, let client.instances().getInstance() return that info to the caller (as we already have it). So no extra network request here.

Basically correct, only that the call would not have to be client.instances().getInstance() in that case, but just client.instance returning the val instance that was created while creating the specific MastodonClient. I'm thinking of it less as caching data to avoid a repeated network request, and more as data that is integral to a specific client object anyway and either doesn't change at all for the typical lifetime of that client object (like version, source_url etc.) or at least is very unlikely to change too often (like description, thumbnail, many of the configuration details, ...).

For what it's worth, I would assume that the typical lifetime of a MastodonClient object is a single user interaction with their Mastodon app that uses our library, so probably minutes or at most hours, not much more than that.

That is really just a minor point, though. If we go for client.instances().getInstance() now and eventually find out that something else would be better, switching to something else in another major release should not cause too much trouble for library users. More than OK for me, if we do that for now. :)

from bigbone.

This keeps being more complicated than expected. :(

I've now implemented the first part, renaming to singular terms and changing the Java getter.

I believe I can implement getting the server instance even while building the client - but at the moment it would have to be the (always available but deprecated) V1 instance because we haven't yet implemented the V2 instance, or even finalized completely how exactly we want to do that.

For what it's worth, if we want to store either the V1 or V2 instance in our MastodonClient, then we would probably have to have an (abstract?) Instance super class containing common fields between the V1 and V2 responses, plus separate InstanceV1 and InstanceV2 sub-classes that each have fields unique to that version. Then, when accessing one of the api/vX/instance endpoints, we would then deserialize into the matching versioned instance class, but have a member of the super class in MastodonClient. In turn, library users would then have to use is/instanceOf when working with that member.

This probably is a viable way to go on about that, but I'm not sure if we should go down that rabbit hole right now. Perhaps we should better just do the first step right now, and talk about the remainder in the context of our V1/V2 API support some more?

from bigbone.

• agreed upon changes were added with #111
• potential rewrite to have an exposed Instance member in MastodonClient was newly added as #114
• comment about Instance super class and InstanceV1/V2 subclasses was added to discussion #13.

Closing this one.

from bigbone.