bigbone icon indicating copy to clipboard operation
bigbone copied to clipboard

Refactoring endpoints and fix some of them

Open G10xy opened this issue 1 year ago • 4 comments

Since I have seen that sometimes the endpoint is set as a private variable within the class to be called within all methods of that class, sometimes it is rewritten in each method, sometimes it is rewritten in the corresponding test class, I was wondering if it might be useful and nicer to group them all in a companion object, so that they can then be called statically when and where needed. What do you think?

Indeed, I have seen that, regarding endpoints, sometimes they start with the slash "/" (for example: /api/v1/endorsements), sometimes they start without the slash "/" (for example: api/v1/accounts). Which structure is the wrong one?

G10xy avatar Dec 20 '23 15:12 G10xy

In MastodonClient.fullUrl(...) we call HttpUrl.Builder().addEncodedPathSegments(path) with the endpoint string as path. According to their documentation, this should probably be done without a leading slash for consistency:

Adds a set of encoded path segments separated by a slash (either \ or /). If encodedPathSegments starts with a slash, the resulting URL will have empty path segment.

I'm not sure if moving the actual endpoint definitions out of the individual method definitions would serve any purpose. With everything (endpoint, method, parameter lists) defined in the same place, it is in my opinion much easier to see what's going on.

bocops avatar Dec 20 '23 16:12 bocops

I have already started cleaning up a bit in #410 but one item of discussion came up:

How do we want to handle endpoints in tests?

I think the cleanest solution with least amount of overhead would be to define the endpoint vals as internal instead of private.

Another one would be to copy the vals over to the tests to be used by all test methods.

Any opinion on that? @bocops @andregasser @G10xy

PattaFeuFeu avatar Dec 30 '23 21:12 PattaFeuFeu

I think setting the endpoint property to internal is a good solution. An endpoint is needed per ...Methods.kt class. Test classes can make use of it if needed (not all tests currently use it). I certainly would not duplicate them.

andregasser avatar Dec 30 '23 21:12 andregasser

I agree with the suggestion made by @andregasser. Keep going with the cleanest solution of setting endpoints as internal 👍🏼

G10xy avatar Jan 03 '24 20:01 G10xy