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

Refactoring endpoints and fix some of them #396

Open
G10xy opened this issue Dec 20, 2023 · 4 comments
Open

Refactoring endpoints and fix some of them #396

G10xy opened this issue Dec 20, 2023 · 4 comments
Assignees

Comments

@G10xy
Copy link
Contributor

G10xy commented Dec 20, 2023

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?

@bocops
Copy link
Collaborator

bocops commented Dec 20, 2023

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.

@PattaFeuFeu
Copy link
Collaborator

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

@andregasser
Copy link
Owner

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.

@G10xy
Copy link
Contributor Author

G10xy commented Jan 3, 2024

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

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

No branches or pull requests

4 participants