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

refactor user service #30

Merged
merged 3 commits into from
Aug 18, 2020
Merged

refactor user service #30

merged 3 commits into from
Aug 18, 2020

Conversation

kunwar97
Copy link
Contributor

No description provided.

@tgiardina
Copy link
Owner

@kunwar97 Dang! You resolved #20, but I think there's a problem with the issue itself! I forgot that these endpoints need to generate and return a token, and generating a token is neither the job of a controller nor of a repository, so it must be the job of a separate service layer. Does that make sense? I'll update the issue ASAP.

Meanwhile, there are still some good changes I would like to merge from this PR. In particular,

  1. Deleting the result helper is still good.
  2. Swtiching src/controllers/user from using result to using a try-catch block is good.

If you copy back all of src/services/user and also switch it from using result to throwing errors, then I should be able to merge this ASAP and close #21.

@kunwar97
Copy link
Contributor Author

@tgiardina sure

@tgiardina
Copy link
Owner

I'm trying to refactor things so that I don't need UserService. Let's put this on hold until I figure that out.

@tgiardina tgiardina merged commit 8d94fb4 into tgiardina:master Aug 18, 2020
@tgiardina
Copy link
Owner

Pushed this through. Thanks @kunwar97!

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

Successfully merging this pull request may close these issues.

2 participants