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: Split request and response handling parts of the code #16

Merged
merged 3 commits into from
Oct 27, 2022
Merged

Conversation

njhill
Copy link
Member

@njhill njhill commented Oct 26, 2022

Motivation

The current code doesn't have a very clear structure, in particular the request handling logic (JSON->PB) is intermingled with the response handling logic (PB->JSON).

Modifications

  • Minor simplifications to the CustomJSONPb.Marshal and parameterMapToJson functions
  • Split out request handing into separate request.go file and corresponding tests into request_test.go (no code changes apart from moving/reordering)

I'm not renaming marshaler.go to response.go in the same PR because git would probably interpret it as a different file (due to the large number of lines removed at the same time).

Result

Clearer code

Motivation

The current code doesn't have a very clear structure, in particular the request handling logic (JSON->PB) is intermingled with the response handling logic (PB->JSON).

Modifications

- Split out request handing into separate request.go file and corresponding tests into request_test.go
- Minor simplifications to the CustomJSONPb.Marshal and parameterMapToJson functions

I'm not renaming marshaler.go to response.go in the same PR because git would probably interpret it as a different file (due to the large number of lines removed at the same time).

Result

Clearer code

Signed-off-by: Nick Hill <nickhill@us.ibm.com>
Signed-off-by: Nick Hill <nickhill@us.ibm.com>
@njhill
Copy link
Member Author

njhill commented Oct 26, 2022

I have the two listed changes arranged in separate commits for ease of reviewing, will squash before we merge.

Signed-off-by: Nick Hill <nickhill@us.ibm.com>
@njhill njhill marked this pull request as ready for review October 26, 2022 23:16
Copy link
Contributor

@chinhuang007 chinhuang007 left a comment

Choose a reason for hiding this comment

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

Looks good!

@njhill njhill merged commit d9501be into main Oct 27, 2022
@njhill njhill deleted the structure branch October 27, 2022 12:48
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