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

reorganize into app/ and server/ #88

Merged
merged 14 commits into from
Jan 8, 2019
Merged

reorganize into app/ and server/ #88

merged 14 commits into from
Jan 8, 2019

Conversation

cainlevy
Copy link
Member

The new organization has three main divisions:

  1. app/ contains domain logic
  2. server/ maps the domain logic into a standard RESTful server using middleware and handlers
  3. lib/ contains a few generalized things that haven't been fully extracted to standalone repositories

@cainlevy
Copy link
Member Author

@mohammed90 Feedback would be appreciated in light of #83. I'm imagining that the gRPC logic would be a sibling of server/.

@mohammed90
Copy link
Contributor

Okay, I'll take a look by next weekend. I'm currently down with flu.

@cainlevy
Copy link
Member Author

Okay, no rush!

@mohammed90
Copy link
Contributor

@cainlevy LGTM. I just have one thought:

In server/sessions/util.go, the getters explicitly rely on *http.Request. In gRPC side of work, I need exactly the same logic, but we don't have *http.Request there. So I copied those functions into the grpc package and adjusted funcs to take context.Context instead of *http.Request, which makes it agnostic to the source of the context.Context. Do you think it's better to refactor this logic to be reusable across both packages in this PR, or do you want to leave it for later when we're reconciling them?

The setter seems to have to be tied to the response type, so I can't think of a way to make it agnostic.

func Set(cfg *app.Config, w http.ResponseWriter, val string) {

@cainlevy
Copy link
Member Author

cainlevy commented Jan 8, 2019

@mohammed90 I'm back from the winter holiday and ready to finish up this refactor!

Do you think it's better to refactor this logic to be reusable across both packages in this PR, or do you want to leave it for later when we're reconciling them?

Let's see how it turns out in the end? The amount of duplication you're describing sounds acceptable to me.

@cainlevy cainlevy merged commit cea0f44 into master Jan 8, 2019
@cainlevy cainlevy deleted the app-and-server branch January 8, 2019 17:47
@coveralls
Copy link

Coverage Status

Coverage increased (+2.3%) to 77.197% when pulling ec4e796 on app-and-server into e57037c on master.

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.

3 participants