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.

Sign up for GitHub

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

Jump to bottom

Extract handler parameters from corresponding structure #2513

Merged
merged 59 commits into from
May 11, 2023
Merged

Extract handler parameters from corresponding structure #2513

merged 59 commits into from
May 11, 2023

Conversation

w84thesun
Copy link
Contributor

@w84thesun w84thesun commented Apr 25, 2023 •
edited
Loading

Description

In that PR I introduce ExtractParams function that fills the parameters structure by parsing its tags.

That change was made only for count handler parameters for now.

Closes #2465.

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@w84thesun w84thesun added code/chore Code maintenance improvements no ci labels Apr 25, 2023
@w84thesun w84thesun self-assigned this Apr 25, 2023
@mergify
Copy link
Contributor

mergify bot commented May 3, 2023

@w84thesun this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label May 3, 2023
Merge branch 'main' into issue-2465-refactor-handlers-params
ff142fc 
# Conflicts:
#	internal/handlers/common/count.go
#	internal/handlers/common/params.go
#	internal/handlers/pg/msg_count.go
@mergify mergify bot removed the conflict PRs that have merge conflicts label May 4, 2023
chilagrow
chilagrow reviewed May 11, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Just few comments about testing 🤗

internal/handlers/commonparams/extract_params.go Show resolved Hide resolved
internal/handlers/commonparams/extract_params.go Outdated Show resolved Hide resolved
internal/handlers/commonparams/extract_params.go Outdated Show resolved Hide resolved
internal/handlers/commonparams/extract_params_test.go Show resolved Hide resolved
@AlekSi AlekSi added this to the Next milestone May 11, 2023
@w84thesun w84thesun requested a review from chilagrow May 11, 2023 08:20
chilagrow
chilagrow approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Great test coverage too!

rumyantseva
rumyantseva approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

Great job! The approach looks consistent to me, so I believe we should merge it.

@w84thesun w84thesun merged commit 7aa13c3 into FerretDB:main May 11, 2023
@w84thesun w84thesun deleted the issue-2465-refactor-handlers-params branch May 11, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@noisersup noisersup Awaiting requested review from noisersup noisersup was automatically assigned from FerretDB/core

@AlekSi AlekSi Awaiting requested review from AlekSi

Assignees

@w84thesun w84thesun

Labels
code/chore Code maintenance improvements
Projects
Current Engineering
Archived in project
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

Refactor how we process handlers params
4 participants
@w84thesun @AlekSi @rumyantseva @chilagrow