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

Move service "movieAddHandler" logic to the manager package #4

Open
lsmoura opened this issue Oct 1, 2021 · 6 comments
Open

Move service "movieAddHandler" logic to the manager package #4

lsmoura opened this issue Oct 1, 2021 · 6 comments

Comments

@lsmoura
Copy link
Owner

lsmoura commented Oct 1, 2021

No description provided.

@thevibhu
Copy link

thevibhu commented Oct 2, 2021

I would like to take up this issue @lsmoura !

@lsmoura
Copy link
Owner Author

lsmoura commented Oct 2, 2021

Sure! Go ahead!!

@thevibhu
Copy link

thevibhu commented Oct 2, 2021

Just to be sure, do I have to only move movieAddHandler method from package service to package manager, or do I have to move other methods as well? After I am done, how can I test my code/changes?

@lsmoura
Copy link
Owner Author

lsmoura commented Oct 2, 2021

Just this one method is fine. The move is actually the first step to make code testable and I’ll write tests next week.

If you want to run the service and call api endpoints using curl, you can do so, but you need some api keys (you can see the env variables on the cmd folder).

You can just make the PR to the best of your ability and I can do some manual testing.

@thevibhu
Copy link

thevibhu commented Oct 3, 2021

Just moving one method to the manager package is making some code repeating or an import cycle as movieAddHandler method has Config value receiver. This can be solved, but to solve the import cycle I have to repeat the Config struct in both packages and still have issues when calling the methods in the main and service file. I think moving other methods as well in the manager package will be better and we can comment on the code in the movie.go as of now. What do you suggest?

@lsmoura
Copy link
Owner Author

lsmoura commented Oct 8, 2021

The idea is to remove the service dependency on the database, sabnzbd and tmdb services. I'm ok with having code duplication until all the methods are moved.

The manager package should not be dependent on the config package. For now, we can pass the required values to the manager function as parameters and then phase out the parameter requirements once the manager has everything it needs, and eventually remove the service dependency from other packages except for the manager and chi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants