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

Remove Content-Type from non-body HTTP methods #253

Merged
merged 1 commit into from Oct 24, 2021
Merged

Remove Content-Type from non-body HTTP methods #253

merged 1 commit into from Oct 24, 2021

Conversation

thicolares
Copy link
Contributor

@thicolares thicolares commented Oct 22, 2021 •
edited

  • The non-body HTTP methods: GET and DELETE.
  • Expose the headers as arg to make it explicit and move the decision/concern to each method instead of send_request.
  • Use keywords args as there're multiple optional ones.
  • Extract the non-body headers to a classes property to convey its intention with its name and reduce duplication.
  • Duly update related test.

Closes the topic 1/3 of #243. It's a small step towards the other topics. Maybe they could even be bundled into a single PR, but I don't think it's mandatory.

Currently, the SDKs always send Content-Type: application/json to every request. Only the POST and PUT requests should send the Content-Type: application/json and not the DELETE and GET ones.

@thicolares
Remove Content-Type from non-body HTTP methods
20b6302 
- GET and DELETE.
- Expose the headers as arg to make it explicit and move the decision/concern to each method instead of send_request.
- Use keywords args as there're 3 optional ones.
- Extract the non-body headers to a property to convey its intention and reduce duplication.
- Duly update related test.
@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Oct 24, 2021
@bors
Try #253:
7b91d0e
@bors
Copy link
Contributor

bors bot commented Oct 24, 2021

try

Build succeeded:

curquiza
curquiza approved these changes Oct 24, 2021
View reviewed changes
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

A really nice change, thank you @thicolares!!

bors merge

If you are participating in Hacktoberfest, and you would like to receive a small gift from MeiliSearch too, please complete this form.

@bors
Copy link
Contributor

bors bot commented Oct 24, 2021

Build succeeded:

@bors bors bot merged commit 836d83b into meilisearch:main Oct 24, 2021
@curquiza curquiza added skip-changelog The PR will not appear in the release changelogs bug Something isn't working and removed skip-changelog The PR will not appear in the release changelogs labels Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curquiza curquiza curquiza approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thicolares @curquiza