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

Chore: Introduce ArtemisContext and ArtemisContextBasedService #405

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

FelberMartin
Copy link
Collaborator

@FelberMartin FelberMartin commented Feb 11, 2025

Problem Description

Currently we have a lot of code duplication in the service implementations. Most prominently, the serverUrl and authToken is passed down to (almost) every single API call. This clutters the code, especially in the ViewModels.

This PR is the first of several planned PRs for introducing the new concept of an ArtemisContext and an ArtemisContextBasedService that does not need a serverUrl or authToken as function parameters. To keep the PR size in a manageable scale, there will be follow-up PRs dealing with the introduction of this ArtemisContext to other services as well. For this first PR, I only applied the necassary changes to the Services defined in the core:data module.

Changes

  • Introduced
    • ArtemisContext: Simple data class to keep track of serverUrl and authToken
    • ArtemisContextProvider: Provides a flow for tracking the latest context
    • ArtemisContextBasedService: Interface used by service definitions to indicate that the serverUrl and authToken are handled internally and not needed as function parameters
    • ArtemisContextBasedServiceImpl: Implementation of the interface above. Provides convenient methods for API requests (taking care of the serverUrl and authToken in the background)

Steps for testing

See the affected services in the commit messages and confirm that related functionality in the app still works

@FelberMartin FelberMartin added the ready for review This PR can be reviewed label Feb 11, 2025
@FelberMartin FelberMartin self-assigned this Feb 11, 2025
Copy link
Contributor

@julian-wls julian-wls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice changes this makes already makes it a lot cleaner. I tried to go through all of the screens of the app and noticed that the pull to refresh is not working anymore in the Exercise Screen, it does not happen on develop. Could you take a look?

Couldn't find issues in your code.

@FelberMartin
Copy link
Collaborator Author

Nice catch, this should be fixed now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR can be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants