Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable get-return lint #6245

Closed
wants to merge 1 commit into from
Closed

Conversation

Rohanraj123
Copy link
Contributor

Which problem is this PR solving?

#5506

Description of the changes

  • Changed functions name get prefix -> extract prefix to bypass the linter

How was this change tested?

  • make lint ``make test

Checklist

Signed-off-by: Rohanraj123 <rajrohan88293@gmail.com>
@Rohanraj123 Rohanraj123 requested a review from a team as a code owner November 23, 2024 08:32
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Sorry, the changes make no sense to me - why would we rename our business methods that match the naming in the external API? It's counterproductive.

@Rohanraj123
Copy link
Contributor Author

Sorry, the changes make no sense to me - why would we rename our business methods that match the naming in the external API? It's counterproductive.

Can splitting the method into two such as :

`func (h *HTTPGateway) getServices(w http.ResponseWriter, r *http.Request) {
	services, err := h.QueryService.GetServices(r.Context())
	if h.tryHandleError(w, err, http.StatusInternalServerError) {
		return
	}
	h.marshalResponse(&api_v3.GetServicesResponse{
		Services: services,
	}, w)
}

`

can be converted into :

`func (h *HTTPGateway) getServices(ctx context.Context) ([]Service, error) {
	return h.QueryService.GetServices(ctx)
}

func (h *HTTPGateway) handleGetServices(w http.ResponseWriter, r *http.Request) {
	services, err := h.getServices(r.Context())
	if h.tryHandleError(w, err, http.StatusInternalServerError) {
		return
	}
	h.marshalResponse(&api_v3.GetServicesResponse{
		Services: services,
	}, w)
}
`

Would this approach work ? Suggestions?

@yurishkuro
Copy link
Member

Discussing approaches means there is a problem to be solved. I don't see a problem with our code structure. Not every linter needs to be turned on.

@Rohanraj123
Copy link
Contributor Author

make sense, I close the PR.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants