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

Hide implementation details from library users #5

Closed
andregasser opened this issue Dec 5, 2022 · 6 comments · Fixed by #52
Closed

Hide implementation details from library users #5

andregasser opened this issue Dec 5, 2022 · 6 comments · Fixed by #52
Assignees
Labels
code quality Everything related to code quality enhancement New feature or request
Milestone

Comments

@andregasser
Copy link
Owner

andregasser commented Dec 5, 2022

OkHttp3 and Gson leak through the client-side API. Check if really necessary and change if needed. Leaking details to the library users makes it difficult to change implementation details without breaking the API.

@andregasser andregasser added the enhancement New feature or request label Dec 12, 2022
@andregasser andregasser modified the milestones: 1.8.0, 2.0.0 Dec 12, 2022
@bocops
Copy link
Collaborator

bocops commented Dec 19, 2022

For some progress here, I have a patch at https://github.com/bocops/mastodon4j/tree/patch_mastodonclient_1 that changes the GET/DELETE/PATCH/PUT methods in MastodonClient to mostly accept Parameter lists instead of having to expose OkHttp's RequestBody.

Should this be added to main directly, or do we want to have a separate branch for that?

@andregasser
Copy link
Owner Author

andregasser commented Dec 19, 2022

Hello @bocops, maybe we should start using feature branches for issues in general, and create PR's for merging them into master when they are ready? Would make reviewing code easier, right?

@andregasser andregasser added the code quality Everything related to code quality label Dec 20, 2022
@andregasser
Copy link
Owner Author

@bocops : Looking at the MastodonClient.Builder()signature, I think we could also get of the Gson() instance that is passed every time the Builder(...) is called. IMHO, we could remove the Gson parameter entirely from the method signature, as we always pass the same object. I do not think that a library user should ever be able to tinker with the serialization/deserialization logic we're relying on. Do you share that opinion?

@bocops
Copy link
Collaborator

bocops commented Dec 24, 2022

Definitely, we shouldn't expose implementation details to users like that, the Gson() instance in each Builder call should be removed and I can deal with that in my upcoming revised PR.

I think we can go one step further. Currently, basically every API method looks like this:

    fun getAccount(accountId: Long): MastodonRequest<Account> {
        return MastodonRequest(
            { client.get("api/v1/accounts/$accountId") },
            { client.getSerializer().fromJson(it, Account::class.java) }
        )
    }

with the Gson instance being exposed via MastodonClient.getSerializer(). It is probably cleaner to change this into something like a call to client.getMastodonRequest(...), which would deal with the necessary serialization in our MastodonClient instead. I would like to do that in a separate PR later, because the one I'm working on already deals with #48 and #49 and would become too massive otherwise.

@andregasser
Copy link
Owner Author

andregasser commented Dec 24, 2022

Yes, sounds good to me. Classes like Accountsprovide the methods for calling the API endpoints (like now) but deserialization is moved into MastodonClient. Doing so, we can save quite some lines of code I guess. I also see no issues with the reactive implementation, as they just call the non-reactive ones. Yes, let's do this in a separate issue/PR then. Thanks for pointing out! 😀

@bocops
Copy link
Collaborator

bocops commented Dec 24, 2022

Reposted the above as separate issue #51.

bocops added a commit to bocops/bigbone that referenced this issue Dec 24, 2022
including API method classes using MastodonClient and related functionality to achieve the following:

- remove OkHttp3 and Gson instances from MastodonClient.Builder as unnecessary exposure of implementation details (closes andregasser#5)

- build all endpoint URLs using OkHttp3.HttpUrl instead of string concatenation (closes andregasser#48)

- remove the MastodonClient.postUrl() special case after endpoint path prefix is no longer hardcoded (closes andregasser#49).
bocops added a commit to bocops/bigbone that referenced this issue Dec 24, 2022
including API method classes using MastodonClient and related functionality to achieve the following:

- remove OkHttp3 and Gson instances from MastodonClient.Builder as unnecessary exposure of implementation details (closes andregasser#5)

- build all endpoint URLs using OkHttp3.HttpUrl instead of string concatenation (closes andregasser#48)

- remove the MastodonClient.postUrl() special case after endpoint path prefix is no longer hardcoded (closes andregasser#49).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Everything related to code quality enhancement New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants